Add notifications settings - #271
Open
isaistvo wants to merge 5 commits into
Open
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughAdds end-to-end notification preference management: a new PostgreSQL ChangesNotification Preferences Feature
Frontend Rebrand and Cosmetic Cleanup
Sequence Diagram(s)sequenceDiagram
participant Browser
participant AccountSettingsPage
participant NotificationAPI
participant NotificationHandler
participant NotificationService
participant Repository
Browser->>AccountSettingsPage: load settings page
AccountSettingsPage->>NotificationAPI: fetchNotificationPreferences(token)
NotificationAPI->>NotificationHandler: GET /api/notifications/preferences
NotificationHandler->>NotificationService: GetPreferences(ctx, recipientID)
NotificationService->>Repository: GetPreferences(ctx, recipientID)
Repository-->>NotificationService: map[string]bool (from JSONB)
NotificationService-->>NotificationHandler: merged defaults + stored prefs
NotificationHandler-->>AccountSettingsPage: JSON preferences
AccountSettingsPage-->>Browser: render Switch toggles
Browser->>AccountSettingsPage: toggle preference
AccountSettingsPage->>NotificationAPI: updateNotificationPreferences(token, prefs)
NotificationAPI->>NotificationHandler: PUT /api/notifications/preferences
NotificationHandler->>NotificationService: UpdatePreferences(ctx, recipientID, prefs)
NotificationService->>Repository: UpdatePreferences (upsert JSONB)
Repository-->>NotificationService: ok
NotificationHandler-->>AccountSettingsPage: 204 No Content
AccountSettingsPage-->>Browser: toast success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
frontend/src/hooks/useRealtimeNotifications.tsx (1)
45-45: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCap notification list growth in memory.
Line 45 prepends indefinitely; long sessions can accumulate a large array and degrade render performance. Keep only the latest N items.
♻️ Suggested change
- setNotifications((prev) => [data, ...prev]); + setNotifications((prev) => [data, ...prev].slice(0, 200));🤖 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/hooks/useRealtimeNotifications.tsx` at line 45, In the setNotifications state update, the notifications array grows indefinitely by prepending each new notification without any limit. This can cause memory issues and performance degradation over long sessions. Modify the callback in setNotifications to cap the array size by keeping only the latest N items (for example, the most recent 100 or 1000 notifications). After prepending the new data to the previous notifications, use array slicing to trim the array to the desired maximum length, ensuring old notifications are discarded when the limit is exceeded.
🤖 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/hooks/useRealtimeNotifications.tsx`:
- Around line 34-35: Remove the access token from the query string in the
EventSource URL construction (line 34 where the url is built with the token
parameter) and instead pass the withCredentials option to the EventSource
constructor to enable cookie-based authentication. Remove any logging that
exposes token fragments from lines 38 and 52. Apply the same changes to the
buildNotificationsStreamUrl function in ui/src/api/business/notifications.ts.
The backend would need to support cookie-based or session-ticket authentication
instead of accepting tokens in query parameters.
In `@frontend/src/pages/guest/Settings/AccountSettingsPage.tsx`:
- Around line 105-131: The Switch component lacks proper accessibility
attributes needed for assistive technologies. Add role="switch" to the button
element to identify it as a switch control, and add aria-checked with the value
of the checked prop to expose the current state to screen readers. This will
ensure that users relying on assistive technologies can properly understand and
interact with the toggle switch at the Switch function level.
In `@frontend/src/utils/audio.ts`:
- Line 5: The AudioContext created with `new AudioContextClass()` at line 5 is
never closed after playback completes, which will exhaust the browser's limit on
concurrent AudioContext instances. Ensure that after the audio playback finishes
(by attaching a listener to the audio end event or after the playback duration),
call the close() method on the `ctx` AudioContext instance to properly release
the resource and prevent context exhaustion.
In `@internal/notification/repository/repository.go`:
- Around line 139-152: The UpdatePreferences method has a vulnerability where
marshaling a nil prefs map produces JSON null, which when merged via the JSONB
|| operator on line 151, can persist a non-object settings value that breaks
downstream map unmarshalling in read operations. Add a nil guard check at the
start of the UpdatePreferences method to validate the prefs parameter is not
nil, and if it is nil, initialize it as an empty map before proceeding with the
json.Marshal call to ensure settings always remains a valid JSON object in the
database.
In `@internal/notification/service/service.go`:
- Around line 149-150: The UpdatePreferences method at line 149 accepts
preference keys from the client without validation, which allows unsupported
keys to be persisted and potentially alter delivery behavior through direct key
lookups. Add validation in the UpdatePreferences method to ensure all keys in
the incoming prefs map are restricted to the known preference key set that is
used in GetPreferences. Only allow updates for valid, supported preference keys
before calling s.repo.UpdatePreferences.
---
Nitpick comments:
In `@frontend/src/hooks/useRealtimeNotifications.tsx`:
- Line 45: In the setNotifications state update, the notifications array grows
indefinitely by prepending each new notification without any limit. This can
cause memory issues and performance degradation over long sessions. Modify the
callback in setNotifications to cap the array size by keeping only the latest N
items (for example, the most recent 100 or 1000 notifications). After prepending
the new data to the previous notifications, use array slicing to trim the array
to the desired maximum length, ensuring old notifications are discarded when the
limit is exceeded.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2678a446-eb40-4370-99c5-2cb54142f1b2
📒 Files selected for processing (16)
frontend/README.mdfrontend/index.htmlfrontend/package.jsonfrontend/src/api/client.tsfrontend/src/api/notifications.tsfrontend/src/components/Notifications/NotificationBell.tsxfrontend/src/hooks/useRealtimeNotifications.tsxfrontend/src/pages/guest/Settings/AccountSettingsPage.tsxfrontend/src/types/api.tsfrontend/src/utils/audio.tsfrontend/tsconfig.app.jsoninternal/business/handler/business/handler.gointernal/notification/handler/handler.gointernal/notification/repository/repository.gointernal/notification/service/service.gomigrations/20260622100000_notification_preferences.sql
Comment on lines
+34
to
+35
| const url = `/api/notifications/stream?access_token=${encodeURIComponent(token)}`; | ||
| const es = new EventSource(url); |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the file
fd -t f "useRealtimeNotifications" --type f | head -5Repository: ua-academy-projects/share-bite
Length of output: 305
🏁 Script executed:
# Get the file structure and read the relevant lines
cat -n frontend/src/hooks/useRealtimeNotifications.tsx | sed -n '30,60p'Repository: ua-academy-projects/share-bite
Length of output: 1603
🏁 Script executed:
# Search for the /api/notifications/stream endpoint in backend
fd -t f "\.ts$" "\.tsx$" "\.js$" | xargs rg "notifications/stream" -l 2>/dev/nullRepository: ua-academy-projects/share-bite
Length of output: 399
🏁 Script executed:
# Check for backend auth patterns (cookies, auth middleware)
rg -t js -t ts "withCredentials|credentials.*include|cookie" --max-count=3 | head -20Repository: ua-academy-projects/share-bite
Length of output: 168
🏁 Script executed:
# Read the backend notifications endpoint implementation
cat -n ui/src/api/business/notifications.ts | head -100Repository: ua-academy-projects/share-bite
Length of output: 1845
🏁 Script executed:
# Search for API route definitions related to notifications/stream
rg "notifications.*stream" -A 10 -B 2Repository: ua-academy-projects/share-bite
Length of output: 168
🏁 Script executed:
# Check for auth middleware patterns
rg "passport|auth.*middleware|withAuth|guards.*auth" -B 2 -A 5 | head -50Repository: ua-academy-projects/share-bite
Length of output: 168
🏁 Script executed:
# Search for the stream endpoint implementation in backend
rg "notifications.*stream" -t ts -t js -B 5 -A 15Repository: ua-academy-projects/share-bite
Length of output: 168
🏁 Script executed:
# Look for EventSource or SSE handling in backend
rg "EventSource|\.on\(|response\.write|stream.*auth" -t ts -t js | head -30Repository: ua-academy-projects/share-bite
Length of output: 168
🏁 Script executed:
# Search for backend route definitions
fd -type f -name "*route*" -o -name "*controller*" | xargs rg "notifications" -l 2>/dev/nullRepository: ua-academy-projects/share-bite
Length of output: 1459
🏁 Script executed:
# Read the backend notification handler
cat -n internal/business/handler/notification/handler.go | head -150Repository: ua-academy-projects/share-bite
Length of output: 2560
🏁 Script executed:
# Also check the other handler
cat -n internal/notification/handler/handler.go | head -150Repository: ua-academy-projects/share-bite
Length of output: 5038
🏁 Script executed:
# Search for how the token is validated in the stream endpoint
rg "access_token|stream" internal/ -t go -B 3 -A 8 | head -80Repository: ua-academy-projects/share-bite
Length of output: 6475
🏁 Script executed:
# Check what authMiddleware expects - look for auth implementation
rg "authMiddleware|middleware.*auth" internal/ cmd/ -B 2 -A 10 | head -100Repository: ua-academy-projects/share-bite
Length of output: 6569
🏁 Script executed:
# Look for cookie/session configuration
rg "cookie|session|SetCookie" internal/ cmd/ -B 2 -A 5 | head -80Repository: ua-academy-projects/share-bite
Length of output: 6144
🏁 Script executed:
# Check how the middleware validates tokens
rg "Bearer|Authorization|GetUserID" internal/middleware -B 3 -A 8Repository: ua-academy-projects/share-bite
Length of output: 2647
Remove bearer token from SSE URL and logging; implement cookie-based or session-ticket authentication.
Line 34 embeds the access token in the query string, and lines 38/52 log token fragments. This increases token exposure through browser logs, proxy logs, and server access logs. While the backend's OptionalAuth middleware currently accepts query parameter tokens as a fallback, this pattern should be phased out in favor of more secure authentication.
Implementation note: The browser's EventSource API cannot send custom headers, so the suggested approach requires backend support for cookie-based authentication. The backend would need to:
- Set a secure, HttpOnly, SameSite cookie on the initial request
- Remove the fallback query-parameter token acceptance from the middleware
- Accept requests with credentials when using
new EventSource(url, { withCredentials: true })
Alternatively, consider using short-lived session tickets or a WebSocket-based approach.
Also applies to: 38-39, 52, and the buildNotificationsStreamUrl function in ui/src/api/business/notifications.ts
🤖 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/hooks/useRealtimeNotifications.tsx` around lines 34 - 35, Remove
the access token from the query string in the EventSource URL construction (line
34 where the url is built with the token parameter) and instead pass the
withCredentials option to the EventSource constructor to enable cookie-based
authentication. Remove any logging that exposes token fragments from lines 38
and 52. Apply the same changes to the buildNotificationsStreamUrl function in
ui/src/api/business/notifications.ts. The backend would need to support
cookie-based or session-ticket authentication instead of accepting tokens in
query parameters.
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.
Closes #60
Summary by CodeRabbit