chore: add ticket attachment in the admin side and feedback types#88
Merged
chore: add ticket attachment in the admin side and feedback types#88
Conversation
…kets and comments
…cket updates; add daphne for the server
…hen improve the admin ticket page; add notification and ws store and types.
…comment data matching and prevent duplicate comments in store
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThis PR implements a real-time notification system with WebSocket support, adding ticket creation notifications, per-user notification groups, Daphne integration, a centralized frontend notification store, and role-based ticket rendering across multiple components. Changes
Sequence DiagramsequenceDiagram
participant Client as Client (Browser)
participant Layout as Root Layout
participant WS as WebSocket Store
participant Server as Server
participant NotifStore as Notification Store
participant NotifAPI as Notifications API
participant UI as UI Component
Client->>Layout: Load page (authenticated)
Layout->>Layout: onMount + subscribe to authStore
Layout->>WS: connect()
WS->>Server: new WebSocket("/ws/tickets/")
Server-->>WS: Connection established
WS->>WS: Set status to "connected"
Note over Client,Server: Ticket Creation Flow
Client->>Server: Create ticket
Server->>Server: notify_ticket_created()
Server->>WS: Broadcast: {action:"created", ticket_id:123, message:"..."}
Note over WS,NotifStore: WebSocket Message Handling
WS->>WS: handleMessage(event)
alt action === "created"
WS->>NotifStore: addNotification({ticket_id, message, ...})
else type === "new_notification"
WS->>NotifStore: addNotification(data.notification)
end
Note over NotifStore,UI: Store Updates UI
NotifStore->>NotifStore: Update notifications array
NotifStore->>NotifStore: Increment unreadCount
UI->>NotifStore: Subscribe to $notificationStore
NotifStore-->>UI: Reactive update
UI->>UI: Render new notification
Note over Client,Server: Mark as Read
Client->>UI: Click mark as read
UI->>NotifStore: markAsRead(id)
NotifStore->>NotifAPI: API call
NotifAPI-->>NotifStore: Success
NotifStore->>NotifStore: Update local state
UI-->>Client: UI reflects change
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…acking currentView; add reloadTickets() logic. Also update tickets’ comments_count in real time via adjustCommentCount()
![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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/components/layout/StudentLayout.svelte (1)
48-57:⚠️ Potential issue | 🔴 CriticalThe
try/catchinhandleMarkAsReadis ineffective becausenotificationStore.markAsReadswallows errors internally.Looking at the store implementation (lines 44–60 in
notification.store.ts), themarkAsReadmethod catches errors from the API call, logs them to console, and returns normally without re-throwing. This means thecatchblock in the component will never fire — any API failures will silently fail with only a console error, andmarkReadErrorwill never be set. The store also has no error state exposed to communicate failures back to the component.The
id: stringtype is correct and matches what the store expects.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/layout/StudentLayout.svelte` around lines 48 - 57, handleMarkAsRead's try/catch never catches failures because notificationStore.markAsRead swallows errors; update the store's markAsRead (in notification.store.ts) to either re-throw the caught error after logging or expose an error state/return a rejected Promise so callers can detect failures, and then keep the component's handleMarkAsRead as-is to rely on the thrown error (or, if you choose the exposed error state approach, read that error from notificationStore in the component and set markReadError accordingly). Ensure the unique symbols: function handleMarkAsRead, method notificationStore.markAsRead, and the markAsRead implementation in notification.store.ts are updated consistently so failures propagate to the UI.
🧹 Nitpick comments (9)
frontend/src/utils/attachment.ts (1)
18-20: Consolidate image extension logic to prevent drift.
isImageFileadds SVG, but extension logic is now split across multiple helpers. Centralize the list to keep behavior consistent.♻️ Suggested consolidation
+const IMAGE_EXTENSIONS = ["jpg", "jpeg", "png", "gif", "webp", "svg"] as const; + export function getFileIcon(url: string): string { const ext = url.split(".").pop()?.toLowerCase() ?? ""; - if (["jpg","jpeg","png","gif","webp"].includes(ext)) return "mdi:image-outline"; + if (IMAGE_EXTENSIONS.includes(ext as (typeof IMAGE_EXTENSIONS)[number])) return "mdi:image-outline"; if (ext === "pdf") return "mdi:file-pdf-box"; return "mdi:file-document-outline"; } @@ export function isImage(url: string): boolean { const ext = url.split(".").pop()?.toLowerCase() ?? ""; - return ["jpg","jpeg","png","gif","webp"].includes(ext); + return IMAGE_EXTENSIONS.includes(ext as (typeof IMAGE_EXTENSIONS)[number]); } @@ export function isImageFile(url: string): boolean { - const ext = url.split(".").pop()?.toLowerCase(); - return ["jpg", "jpeg", "png", "gif", "webp", "svg"].includes(ext || ""); + const ext = url.split(".").pop()?.toLowerCase() ?? ""; + return IMAGE_EXTENSIONS.includes(ext as (typeof IMAGE_EXTENSIONS)[number]); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/utils/attachment.ts` around lines 18 - 20, The image-extension list in isImageFile is duplicated elsewhere and risks drifting; extract a single shared constant (e.g., IMAGE_EXTENSIONS) and replace the hardcoded array in isImageFile with a reference to that constant, then update any other helpers that compute or check image extensions to use IMAGE_EXTENSIONS as well so all logic is centralized and consistent (refer to the isImageFile function name and any other extension-check helpers you find to replace their arrays).backend/apps/notifications/utils.py (1)
121-121: Scope recipient query to active staff and minimal fields.At Line 121, consider filtering inactive users out and reducing fetched columns.
♻️ Suggested refinement
- for admin_user in User.objects.filter(is_staff=True): + for admin_user in User.objects.filter(is_staff=True, is_active=True).only("id"):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/apps/notifications/utils.py` at line 121, The loop currently iterates over User.objects.filter(is_staff=True) and pulls full user rows; narrow it to active staff and fetch only needed fields to reduce DB load: change the queryset used in the for loop (the User model iteration) to filter is_staff=True and is_active=True and use a minimal-field projection (e.g., only(...) or values/values_list for 'id' and 'email' or whatever fields the loop actually needs) so the code iterates over only active admin users with minimal fetched columns.frontend/src/types/websocket.ts (1)
3-10: Replaceanywith actual types inWSMessage.Using
anyremoves compile-time checks even thoughstrict: trueis enabled. Thecommentfield is accessed with.idand.messageproperties, andnotificationis passed tonotificationStore.addNotification()which expects theNotificationtype. Instead of inline type definitions, import and use the existing types:import type { TicketComment } from './comments'; import type { Notification } from './notifications'; export type WSMessage = { action?: string; type?: string; ticket_id?: number; comment?: TicketComment; message?: string; notification?: Notification; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/types/websocket.ts` around lines 3 - 10, Update the WSMessage type so it doesn't use any: import the TicketComment and Notification types and set comment?: TicketComment and notification?: Notification in the exported WSMessage type; locate the WSMessage declaration and add the two type imports (import type { TicketComment } from './comments' and import type { Notification } from './notifications') and replace the comment and notification fields' any types accordingly so usages that access comment.id/comment.message and notificationStore.addNotification() are type-checked.frontend/src/stores/notification.store.ts (1)
36-41: MakeaddNotificationidempotent by notification ID.Right now repeated delivery of the same event will duplicate items and inflate unread counts.
♻️ Proposed dedupe update
addNotification(notification: Notification) { update((s) => ({ - notifications: [notification, ...s.notifications], - unreadCount: s.unreadCount + (notification.read ? 0 : 1), - isLoading: false, - })); + ...s, + notifications: s.notifications.some((n) => n.id === notification.id) + ? s.notifications + : [notification, ...s.notifications], + unreadCount: s.notifications.some((n) => n.id === notification.id) + ? s.unreadCount + : s.unreadCount + (notification.read ? 0 : 1), + isLoading: false, + })); },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/stores/notification.store.ts` around lines 36 - 41, The addNotification function currently always prepends the incoming Notification, causing duplicates and wrong unreadCount; make it idempotent by checking notification.id against existing s.notifications: if an entry with the same id exists, update/replace that entry (do not prepend another) and adjust unreadCount by the delta between previous.read and notification.read (e.g., if previously read=false and now read=true decrement unreadCount, vice versa), otherwise prepend the new notification and increment unreadCount if notification.read is false; always set isLoading = false. Use the addNotification function name and the notifications/unreadCount fields to locate and change the logic.frontend/src/stores/websocket.store.ts (2)
45-47: Minor:onmessagewrapper is unnecessary.The anonymous wrapper around
handleMessageadds no value.♻️ Simplification
- socket.onmessage = (event) => { - handleMessage(event); - }; + socket.onmessage = handleMessage;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/stores/websocket.store.ts` around lines 45 - 47, Replace the unnecessary anonymous wrapper assignment to socket.onmessage with a direct reference to the handler: remove the lambda that calls handleMessage(event) and assign socket.onmessage = handleMessage so the existing handleMessage function is used directly.
70-116: Message routing uses independentifchains — a single message could trigger both ticket and comment handlers.
data.action(Lines 76–86) anddata.type(Lines 89–111) are checked with separateif/else ifblocks. If the backend ever sends a message with both fields set, both blocks execute. If this is intentional, a brief comment clarifying the design would help. If not, consider usingelse ifor an early return after the first matched handler.Also, the
feedback_createdblock (Lines 104–106) is empty — either add a TODO comment or remove it to avoid dead code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/stores/websocket.store.ts` around lines 70 - 116, The handleMessage function currently evaluates data.action and data.type in independent if chains which allows one incoming message to trigger both ticket and comment handlers; change the routing so only one handler runs per message by making the second block an else-if chain or returning early after handling the first matched case (reference handleMessage, the data.action checks and data.type checks), and add a clear TODO comment or implement behavior for the empty feedback_created branch (the feedback_created block) to avoid dead code; ensure calls to ticketsStore (loadTickets, loadTicketById), commentsStore (addCommentToStore, updateCommentInStore, removeCommentFromStore) and notificationStore.addNotification remain reachable under the correct single-path routing.frontend/src/stores/comment.store.ts (1)
122-146:deleteCommentdoesn't use thefinallypattern forisLoading.
createCommentandupdateCommentwere refactored to usefinallyfor resettingisLoading, butdeleteCommentstill sets it manually in bothtryandcatchbranches. Consider aligning it for consistency.♻️ Suggested refactor
try { await apiDeleteComment(ticketId, commentId); update((state) => ({ ...state, comments: state.comments.filter((comment) => comment.id !== commentId), - isLoading: false, error: null, })); return true; } catch (error) { const errorMessage = error instanceof Error ? error.message : `Failed to delete comment ${commentId}`; console.error(`Error deleting comment ${commentId}:`, error); update((state) => ({ ...state, - isLoading: false, error: errorMessage, })); return false; + } finally { + update((state) => ({ ...state, isLoading: false })); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/stores/comment.store.ts` around lines 122 - 146, The deleteComment function duplicates isLoading state updates in both try and catch; refactor deleteComment to set isLoading true at start, perform apiDeleteComment and state updates in the try, keep the error handling as-is in catch, and move the update that sets isLoading: false into a finally block so isLoading is always reset once (reference function: deleteComment; state updates that filter comments and the catch block that sets error should remain, only remove the explicit isLoading:false assignments from try/catch and put a single update(...) in finally to set isLoading:false).backend/apps/tickets/consumers.py (1)
29-42: All fivesend_*methods are identical — consider a single dispatcher.
send_ticket_update,send_comment_update,send_feedback_update,send_notification, andsend_status_updateall have the exact same body. You could DRY this with a single helper, though you'd still need distinct method names for Channel layer routing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/apps/tickets/consumers.py` around lines 29 - 42, The five identical methods send_ticket_update, send_comment_update, send_feedback_update, send_notification, and send_status_update should delegate to a single helper to avoid duplication: add a private helper method (e.g., _send_event or send_event) that does await self.send(text_data=json.dumps(event["data"])), then update each of the five methods to simply call and return that helper; keep the original method names intact so channel layer routing still resolves to them.frontend/src/components/ui/admin/AttachmentModal.svelte (1)
29-34: Mixed Svelte 4 and Svelte 5 event syntax.Line 27 uses
on:keydown(Svelte 4 directive), while Lines 30, 41, and 64 useonclick(Svelte 5 attribute syntax). Pick one style for consistency within the component. Since the rest of the component uses Svelte 5 style, switch the window handler too:-<svelte:window on:keydown={handleKeydown} /> +<svelte:window onkeydown={handleKeydown} />Also applies to: 41-41, 64-64
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/ui/admin/AttachmentModal.svelte` around lines 29 - 34, The component mixes Svelte 4 and Svelte 5 event syntax — change the Svelte 4 window handler using on:keydown to the Svelte 5 attribute style to match the rest of the file (the buttons already use onclick). Locate the window keydown handler (the place where on:keydown is used) and replace it with the Svelte 5 window event attribute (e.g., onwindow:keydown={...}) and keep existing handler function names like open so all event bindings (onclick={open}, onclick={close}, etc.) use the same Svelte 5 syntax.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/apps/tickets/consumers.py`:
- Around line 22-27: The disconnect() method accesses self.user_group which may
not be set if connect() failed; initialize self.user_group to None (e.g., in the
consumer's __init__ or at top of connect()) or guard its use in disconnect() by
checking hasattr(self, "user_group") or if self.user_group is not None before
calling channel_layer.group_discard; update the disconnect() logic to only call
await self.channel_layer.group_discard(self.user_group, self.channel_name) when
self.user_group is present to avoid AttributeError and still remove the other
groups (ticket_updates, comment_updates, feedback_updates).
- Around line 6-20: The connect method currently joins global groups and accepts
the WebSocket before verifying authentication; move the authentication check
earlier and reject anonymous connections: retrieve user from self.scope
(self.scope.get("user")) at the top of connect, and if user is None or not
user.is_authenticated call await self.close() (or close with an appropriate
code) and return so no group_add or accept occurs; only when user is
authenticated proceed to group_add for "ticket_updates", "comment_updates",
"feedback_updates", and the per-user group (self.user_group = f"user_{user.id}")
and finally call await self.accept().
In `@backend/apps/tickets/views.py`:
- Around line 408-413: The payload currently includes commenter PII by setting
"email": comment.user.email in the user object for WebSocket ticket_updates
events; remove the "email" property from the user dict in the comment_created,
comment_updated, and comment_deleted payloads in backend/apps/tickets/views.py
(where the user dict is constructed for comment objects) so the user object only
contains id, name (getattr(comment.user, "name", None)), and avatar
(getattr(comment.user, "avatar", None)); ensure no other references to
comment.user.email remain in those broadcast payload-building blocks.
- Around line 189-195: Wrap the notify_ticket_created call in a try/except
around the post-write notification so notification errors don't bubble up and
break the successful ticket creation; specifically, in the view where the ticket
is saved and notify_ticket_created(...) is invoked, catch Exception around that
function, log the failure with context (use the existing logger or request
logger and include ticket_obj.id/ticket_number) and do not re-raise—optionally
enqueue/send the notification asynchronously or schedule a retry instead of
failing the response.
In `@backend/requirements.txt`:
- Around line 13-48: Add the missing channels-redis package to requirements by
appending a pinned dependency (e.g. channels-redis==4.0.0) to
backend/requirements.txt so the production channel layer
(channels_redis.core.RedisChannelLayer referenced in settings.py) can be
imported; after updating requirements, reinstall/update your environment (pip
install -r requirements.txt) or update the lockfile as appropriate.
In `@frontend/src/components/ui/admin/AttachmentModal.svelte`:
- Around line 22-27: The Escape handler is always bound via <svelte:window>, so
update it to only act when the modal is open: inside handleKeydown(e:
KeyboardEvent) check the modal state (isOpen) and return early if false, or
alternatively move the <svelte:window on:keydown={handleKeydown} /> inside a
{`#if` isOpen} block so the handler is only registered while the modal is open;
reference the existing handleKeydown, isOpen and close symbols so the
guard/unbinding correctly prevents the handler from running when the modal is
closed.
In `@frontend/src/components/ui/tickets/TicketBoard.svelte`:
- Around line 154-254: The two role branches are identical; remove the `{`#if`
role === "student"} ... {:else if role === "admin"} ... {/if}` split and replace
with a single role-agnostic card block that uses the same markup and handlers.
Keep the existing attributes and handlers (onclick={() => navigate(report)},
onkeydown handling Enter/Space), and retain usage of report,
statusConfig[report.status], priorityConfig[getPriorityKey(report.priority)],
and ticket_number; ensure navigate(report), getPriorityKey, statusConfig and
priorityConfig remain referenced exactly as before. This collapses duplication
while preserving behavior that varies only inside navigate().
- Around line 37-43: The navigate function currently returns silently when role
is null causing a race-case no-op; update navigate(t: Ticket) to handle a
null/undefined role by adding a fallback branch that either logs a warning
(e.g., console.warn("navigate called before auth resolved")) and/or redirects to
a safe default route (e.g., `/login` or `/tickets`) to provide feedback; also
confirm intended admin behavior and either change the admin branch in navigate
to goto(`/tickets/${t.ticket_number}`) so admins open the ticket detail like
students (if admin should see details and AdminTicketControl is rendered there),
or keep the existing goto(`/tickets`) but document that choice and ensure clicks
are disabled while auth isLoading—refer to the navigate function, the role
variable and goto calls when making these changes.
In `@frontend/src/routes/`+layout.svelte:
- Around line 16-21: The connect logic in wsStore can create duplicate sockets
because it only checks for WebSocket.OPEN; update the connect function
(wsStore.connect) to also guard against WebSocket.CONNECTING by checking
socket?.readyState === WebSocket.CONNECTING (in addition to OPEN) and
early-return with a log/message if true, so a pending connection attempt won't
spawn another socket; ensure this check is placed before any new WebSocket(...)
creation and that disconnect/connect callers (like the subscription in
+layout.svelte) can rely on this guard.
In `@frontend/src/stores/websocket.store.ts`:
- Around line 22-26: The connect() guard only checks for WebSocket.OPEN and can
allow a second socket to be created while an earlier socket is still CONNECTING;
update the guard in connect() to return early when socket exists and its
readyState is either WebSocket.OPEN or WebSocket.CONNECTING (i.e., treat
CONNECTING as already in-flight) so you don't create duplicate WebSocket
instances or orphan the in-progress socket; reference the socket variable and
socket.readyState in the connect() function and use WebSocket.CONNECTING in the
condition, keeping the existing onclose/reconnect logic unchanged.
---
Outside diff comments:
In `@frontend/src/components/layout/StudentLayout.svelte`:
- Around line 48-57: handleMarkAsRead's try/catch never catches failures because
notificationStore.markAsRead swallows errors; update the store's markAsRead (in
notification.store.ts) to either re-throw the caught error after logging or
expose an error state/return a rejected Promise so callers can detect failures,
and then keep the component's handleMarkAsRead as-is to rely on the thrown error
(or, if you choose the exposed error state approach, read that error from
notificationStore in the component and set markReadError accordingly). Ensure
the unique symbols: function handleMarkAsRead, method
notificationStore.markAsRead, and the markAsRead implementation in
notification.store.ts are updated consistently so failures propagate to the UI.
---
Nitpick comments:
In `@backend/apps/notifications/utils.py`:
- Line 121: The loop currently iterates over User.objects.filter(is_staff=True)
and pulls full user rows; narrow it to active staff and fetch only needed fields
to reduce DB load: change the queryset used in the for loop (the User model
iteration) to filter is_staff=True and is_active=True and use a minimal-field
projection (e.g., only(...) or values/values_list for 'id' and 'email' or
whatever fields the loop actually needs) so the code iterates over only active
admin users with minimal fetched columns.
In `@backend/apps/tickets/consumers.py`:
- Around line 29-42: The five identical methods send_ticket_update,
send_comment_update, send_feedback_update, send_notification, and
send_status_update should delegate to a single helper to avoid duplication: add
a private helper method (e.g., _send_event or send_event) that does await
self.send(text_data=json.dumps(event["data"])), then update each of the five
methods to simply call and return that helper; keep the original method names
intact so channel layer routing still resolves to them.
In `@frontend/src/components/ui/admin/AttachmentModal.svelte`:
- Around line 29-34: The component mixes Svelte 4 and Svelte 5 event syntax —
change the Svelte 4 window handler using on:keydown to the Svelte 5 attribute
style to match the rest of the file (the buttons already use onclick). Locate
the window keydown handler (the place where on:keydown is used) and replace it
with the Svelte 5 window event attribute (e.g., onwindow:keydown={...}) and keep
existing handler function names like open so all event bindings (onclick={open},
onclick={close}, etc.) use the same Svelte 5 syntax.
In `@frontend/src/stores/comment.store.ts`:
- Around line 122-146: The deleteComment function duplicates isLoading state
updates in both try and catch; refactor deleteComment to set isLoading true at
start, perform apiDeleteComment and state updates in the try, keep the error
handling as-is in catch, and move the update that sets isLoading: false into a
finally block so isLoading is always reset once (reference function:
deleteComment; state updates that filter comments and the catch block that sets
error should remain, only remove the explicit isLoading:false assignments from
try/catch and put a single update(...) in finally to set isLoading:false).
In `@frontend/src/stores/notification.store.ts`:
- Around line 36-41: The addNotification function currently always prepends the
incoming Notification, causing duplicates and wrong unreadCount; make it
idempotent by checking notification.id against existing s.notifications: if an
entry with the same id exists, update/replace that entry (do not prepend
another) and adjust unreadCount by the delta between previous.read and
notification.read (e.g., if previously read=false and now read=true decrement
unreadCount, vice versa), otherwise prepend the new notification and increment
unreadCount if notification.read is false; always set isLoading = false. Use the
addNotification function name and the notifications/unreadCount fields to locate
and change the logic.
In `@frontend/src/stores/websocket.store.ts`:
- Around line 45-47: Replace the unnecessary anonymous wrapper assignment to
socket.onmessage with a direct reference to the handler: remove the lambda that
calls handleMessage(event) and assign socket.onmessage = handleMessage so the
existing handleMessage function is used directly.
- Around line 70-116: The handleMessage function currently evaluates data.action
and data.type in independent if chains which allows one incoming message to
trigger both ticket and comment handlers; change the routing so only one handler
runs per message by making the second block an else-if chain or returning early
after handling the first matched case (reference handleMessage, the data.action
checks and data.type checks), and add a clear TODO comment or implement behavior
for the empty feedback_created branch (the feedback_created block) to avoid dead
code; ensure calls to ticketsStore (loadTickets, loadTicketById), commentsStore
(addCommentToStore, updateCommentInStore, removeCommentFromStore) and
notificationStore.addNotification remain reachable under the correct single-path
routing.
In `@frontend/src/types/websocket.ts`:
- Around line 3-10: Update the WSMessage type so it doesn't use any: import the
TicketComment and Notification types and set comment?: TicketComment and
notification?: Notification in the exported WSMessage type; locate the WSMessage
declaration and add the two type imports (import type { TicketComment } from
'./comments' and import type { Notification } from './notifications') and
replace the comment and notification fields' any types accordingly so usages
that access comment.id/comment.message and notificationStore.addNotification()
are type-checked.
In `@frontend/src/utils/attachment.ts`:
- Around line 18-20: The image-extension list in isImageFile is duplicated
elsewhere and risks drifting; extract a single shared constant (e.g.,
IMAGE_EXTENSIONS) and replace the hardcoded array in isImageFile with a
reference to that constant, then update any other helpers that compute or check
image extensions to use IMAGE_EXTENSIONS as well so all logic is centralized and
consistent (refer to the isImageFile function name and any other extension-check
helpers you find to replace their arrays).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
backend/apps/notifications/utils.pybackend/apps/tickets/consumers.pybackend/apps/tickets/views.pybackend/config/settings.pybackend/requirements.txtfrontend/src/components/layout/AdminLayout.sveltefrontend/src/components/layout/StudentLayout.sveltefrontend/src/components/ui/admin/AdminTicket.sveltefrontend/src/components/ui/admin/AttachmentModal.sveltefrontend/src/components/ui/comments/CommentItem.sveltefrontend/src/components/ui/student/StudentTicket.sveltefrontend/src/components/ui/tickets/CommunityTicketCard.sveltefrontend/src/components/ui/tickets/TicketBoard.sveltefrontend/src/routes/+layout.sveltefrontend/src/stores/comment.store.tsfrontend/src/stores/notification.store.tsfrontend/src/stores/websocket.store.tsfrontend/src/types/comments.d.tsfrontend/src/types/feedback.d.tsfrontend/src/types/notifications.tsfrontend/src/types/tickets.d.tsfrontend/src/types/websocket.tsfrontend/src/utils/attachment.ts
Comment on lines
37
to
43
| function navigate(t: Ticket) { | ||
| goto(`/tickets/${t.ticket_number}`); | ||
| if (role === "admin") { | ||
| goto(`/tickets`); | ||
| } else if (role === "student") { | ||
| goto(`/tickets/${t.ticket_number}`); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's explore the basic structure
find . -type f -name "*.svelte" -o -name "+page.svelte" | grep -E "routes|tickets" | head -20Repository: Polqt/iComplain
Length of output: 1101
🏁 Script executed:
#!/bin/bash
# Check for ticket-related routes
fd --type f -path "*routes*" -name "*ticket*" 2>/dev/null
fd --type f -path "*routes*" \( -name "+page.svelte" -o -name "+layout.svelte" \) | grep ticketRepository: Polqt/iComplain
Length of output: 227
🏁 Script executed:
#!/bin/bash
# Look for role-based navigation patterns and admin routes
rg -rn "role.*admin|admin.*role" --type svelte --type ts | head -30
rg -rn "/tickets" --type svelte --type ts -g '!**/node_modules/**' | grep -v "TicketBoard" | head -20Repository: Polqt/iComplain
Length of output: 120
🏁 Script executed:
#!/bin/bash
# List the routes directory structure
find ./frontend/src/routes -type f -name "+page.svelte" -o -name "+layout.svelte" | sortRepository: Polqt/iComplain
Length of output: 829
🏁 Script executed:
#!/bin/bash
# Search for ticket routes specifically
find ./frontend/src/routes -type d -name "*ticket*"
find ./frontend/src/routes -type f \( -name "*ticket*" \)Repository: Polqt/iComplain
Length of output: 88
🏁 Script executed:
#!/bin/bash
# Look at the TicketBoard.svelte file to understand role initialization
head -80 ./frontend/src/components/ui/tickets/TicketBoard.svelteRepository: Polqt/iComplain
Length of output: 2340
🏁 Script executed:
#!/bin/bash
# Search for authStore and role assignment
grep -rn "authStore\|role.*=\|let.*role" ./frontend/src --include="*.svelte" --include="*.ts" | head -40Repository: Polqt/iComplain
Length of output: 3966
🏁 Script executed:
#!/bin/bash
# Check the ticket detail page
cat -n ./frontend/src/routes/tickets/\[id\]/+page.svelte | head -100Repository: Polqt/iComplain
Length of output: 4265
🏁 Script executed:
#!/antml:thinking
#!/bin/bash
# Look at how ticket detail is accessed and by whom
grep -rn "tickets/\[id\]\|goto.*tickets.*id" ./frontend/src --include="*.svelte" --include="*.ts" | head -30Repository: Polqt/iComplain
Length of output: 41
🏁 Script executed:
#!/bin/bash
# Check the AdminTicketControl component to see if it shows detail
head -50 ./frontend/src/components/ui/tickets/AdminTicketControl.svelteRepository: Polqt/iComplain
Length of output: 1291
🏁 Script executed:
#!/antml:thinking
#!/bin/bash
# Look at the actual routes to see if there's role-based access control
ls -la ./frontend/src/routes/tickets/
find ./frontend/src/routes/tickets -type fRepository: Polqt/iComplain
Length of output: 376
🏁 Script executed:
#!/bin/bash
# Check authStore initialization to understand if role could be undefined initially
find ./frontend/src/stores -name "*auth*"
head -100 ./frontend/src/stores/auth.store.tsRepository: Polqt/iComplain
Length of output: 2756
🏁 Script executed:
#!/bin/bash
# Final verification: check if there are other places where admins navigate to ticket detail
rg -n "goto.*tickets.*\[id\]|goto.*ticket_number" ./frontend/src --type-list | head -20Repository: Polqt/iComplain
Length of output: 448
🏁 Script executed:
#!/bin/bash
# Check if the navigate function is used elsewhere or has related patterns
rg -B5 -A5 "function navigate" ./frontend/src/components/ui/tickets/TicketBoard.svelteRepository: Polqt/iComplain
Length of output: 316
Add fallback for uninitialized role; confirm admin should skip ticket detail.
Two substantiated concerns:
-
Silent no-op on race condition:
authStoreinitializes withrole: nullandisLoading: true. During the asynccheckAuth()window, clicking a card callsnavigate(t)withrole === null, returning silently without routing or feedback. Add a fallback branch (e.g.,console.warnor default route) to handle this. -
Admin navigation discrepancy: Admin navigates to
/tickets(list view), but the detail route at/tickets/[id]renders both admin and student layouts with admin-specific controls (AdminTicketControl). This suggests the detail view is available to admins. Confirm whether admins should navigate to the specific ticket detail like students do, or if the list-only behavior is intentional.
🛡️ Proposed fix — add fallback branch
function navigate(t: Ticket) {
if (role === "admin") {
goto(`/tickets`);
} else if (role === "student") {
goto(`/tickets/${t.ticket_number}`);
+ } else {
+ console.warn("navigate: unrecognized role", role);
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/components/ui/tickets/TicketBoard.svelte` around lines 37 - 43,
The navigate function currently returns silently when role is null causing a
race-case no-op; update navigate(t: Ticket) to handle a null/undefined role by
adding a fallback branch that either logs a warning (e.g.,
console.warn("navigate called before auth resolved")) and/or redirects to a safe
default route (e.g., `/login` or `/tickets`) to provide feedback; also confirm
intended admin behavior and either change the admin branch in navigate to
goto(`/tickets/${t.ticket_number}`) so admins open the ticket detail like
students (if admin should see details and AdminTicketControl is rendered there),
or keep the existing goto(`/tickets`) but document that choice and ensure clicks
are disabled while auth isLoading—refer to the navigate function, the role
variable and goto calls when making these changes.
Comment on lines
+154
to
+254
| {#if role === "student"} | ||
| <div | ||
| role="button" | ||
| tabindex="0" | ||
| aria-label={`Open ticket ${report.title}`} | ||
| onclick={() => navigate(report)} | ||
| onkeydown={(e) => { | ||
| if (e.key === "Enter" || e.key === " ") { | ||
| e.preventDefault(); | ||
| navigate(report); | ||
| } | ||
| }} | ||
| class="card bg-base-100 shadow-sm hover:shadow-md transition-all duration-200 border border-base-content/5 cursor-pointer hover:border-primary/20" | ||
| > | ||
| <div class="card-body p-4"> | ||
| <div class="flex items-center gap-4"> | ||
| <div | ||
| class="badge badge-sm whitespace-nowrap {statusConfig[ | ||
| report.status | ||
| ].color}" | ||
| > | ||
| {statusConfig[report.status].label} | ||
| </div> | ||
|
|
||
| <div class="flex-1 min-w-0"> | ||
| <h3 | ||
| class="font-semibold text-sm text-base-content truncate mb-1" | ||
| <div class="flex-1 min-w-0"> | ||
| <h3 | ||
| class="font-semibold text-sm text-base-content truncate mb-1" | ||
| > | ||
| {report.title} | ||
| </h3> | ||
| <p class="text-xs text-base-content/60"> | ||
| {report.description} | ||
| </p> | ||
| </div> | ||
|
|
||
| <span | ||
| class="text-[10px] font-mono text-base-content/30 shrink-0" | ||
| >{report.ticket_number}</span | ||
| > | ||
| {report.title} | ||
| </h3> | ||
| <p class="text-xs text-base-content/60">{report.description}</p> | ||
|
|
||
| <div | ||
| class="badge badge-sm {priorityConfig[ | ||
| getPriorityKey(report.priority) | ||
| ].color}" | ||
| > | ||
| {priorityConfig[getPriorityKey(report.priority)].label} | ||
| </div> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| {:else if role === "admin"} | ||
| <div | ||
| role="button" | ||
| tabindex="0" | ||
| aria-label={`Open ticket ${report.title}`} | ||
| onclick={() => navigate(report)} | ||
| onkeydown={(e) => { | ||
| if (e.key === "Enter" || e.key === " ") { | ||
| e.preventDefault(); | ||
| navigate(report); | ||
| } | ||
| }} | ||
| class="card bg-base-100 shadow-sm hover:shadow-md transition-all duration-200 border border-base-content/5 cursor-pointer hover:border-primary/20" | ||
| > | ||
| <div class="card-body p-4"> | ||
| <div class="flex items-center gap-4"> | ||
| <div | ||
| class="badge badge-sm whitespace-nowrap {statusConfig[ | ||
| report.status | ||
| ].color}" | ||
| > | ||
| {statusConfig[report.status].label} | ||
| </div> | ||
|
|
||
| <span class="text-[10px] font-mono text-base-content/30 shrink-0" | ||
| >{report.ticket_number}</span | ||
| > | ||
| <div class="flex-1 min-w-0"> | ||
| <h3 | ||
| class="font-semibold text-sm text-base-content truncate mb-1" | ||
| > | ||
| {report.title} | ||
| </h3> | ||
| <p class="text-xs text-base-content/60"> | ||
| {report.description} | ||
| </p> | ||
| </div> | ||
|
|
||
| <span | ||
| class="text-[10px] font-mono text-base-content/30 shrink-0" | ||
| >{report.ticket_number}</span | ||
| > | ||
|
|
||
| <div | ||
| class="badge badge-sm {priorityConfig[ | ||
| getPriorityKey(report.priority) | ||
| ].color}" | ||
| > | ||
| {priorityConfig[getPriorityKey(report.priority)].label} | ||
| <div | ||
| class="badge badge-sm {priorityConfig[ | ||
| getPriorityKey(report.priority) | ||
| ].color}" | ||
| > | ||
| {priorityConfig[getPriorityKey(report.priority)].label} | ||
| </div> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| {/if} |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove the redundant role branches — both blocks are identical.
The {#if role === "student"} ... {:else if role === "admin"} split in the list view produces exactly the same markup, CSS classes, and event handlers in both branches. The only behavioral difference between roles already lives in navigate(), so this template branching is pure duplication.
♻️ Proposed refactor — single card block, role-agnostic template
{`#each` column.reports as report}
- {`#if` role === "student"}
- <div
- role="button"
- tabindex="0"
- aria-label={`Open ticket ${report.title}`}
- onclick={() => navigate(report)}
- onkeydown={(e) => {
- if (e.key === "Enter" || e.key === " ") {
- e.preventDefault();
- navigate(report);
- }
- }}
- class="card bg-base-100 shadow-sm hover:shadow-md transition-all duration-200 border border-base-content/5 cursor-pointer hover:border-primary/20"
- >
- <div class="card-body p-4">
- <div class="flex items-center gap-4">
- <div class="badge badge-sm whitespace-nowrap {statusConfig[report.status].color}">
- {statusConfig[report.status].label}
- </div>
- <div class="flex-1 min-w-0">
- <h3 class="font-semibold text-sm text-base-content truncate mb-1">{report.title}</h3>
- <p class="text-xs text-base-content/60">{report.description}</p>
- </div>
- <span class="text-[10px] font-mono text-base-content/30 shrink-0">{report.ticket_number}</span>
- <div class="badge badge-sm {priorityConfig[getPriorityKey(report.priority)].color}">
- {priorityConfig[getPriorityKey(report.priority)].label}
- </div>
- </div>
- </div>
- </div>
- {:else if role === "admin"}
- <div
- role="button"
- tabindex="0"
- aria-label={`Open ticket ${report.title}`}
- onclick={() => navigate(report)}
- onkeydown={(e) => {
- if (e.key === "Enter" || e.key === " ") {
- e.preventDefault();
- navigate(report);
- }
- }}
- class="card bg-base-100 shadow-sm hover:shadow-md transition-all duration-200 border border-base-content/5 cursor-pointer hover:border-primary/20"
- >
- <div class="card-body p-4">
- <div class="flex items-center gap-4">
- <div class="badge badge-sm whitespace-nowrap {statusConfig[report.status].color}">
- {statusConfig[report.status].label}
- </div>
- <div class="flex-1 min-w-0">
- <h3 class="font-semibold text-sm text-base-content truncate mb-1">{report.title}</h3>
- <p class="text-xs text-base-content/60">{report.description}</p>
- </div>
- <span class="text-[10px] font-mono text-base-content/30 shrink-0">{report.ticket_number}</span>
- <div class="badge badge-sm {priorityConfig[getPriorityKey(report.priority)].color}">
- {priorityConfig[getPriorityKey(report.priority)].label}
- </div>
- </div>
- </div>
- </div>
- {/if}
+ <div
+ role="button"
+ tabindex="0"
+ aria-label={`Open ticket ${report.title}`}
+ onclick={() => navigate(report)}
+ onkeydown={(e) => {
+ if (e.key === "Enter" || e.key === " ") {
+ e.preventDefault();
+ navigate(report);
+ }
+ }}
+ class="card bg-base-100 shadow-sm hover:shadow-md transition-all duration-200 border border-base-content/5 cursor-pointer hover:border-primary/20"
+ >
+ <div class="card-body p-4">
+ <div class="flex items-center gap-4">
+ <div class="badge badge-sm whitespace-nowrap {statusConfig[report.status].color}">
+ {statusConfig[report.status].label}
+ </div>
+ <div class="flex-1 min-w-0">
+ <h3 class="font-semibold text-sm text-base-content truncate mb-1">{report.title}</h3>
+ <p class="text-xs text-base-content/60">{report.description}</p>
+ </div>
+ <span class="text-[10px] font-mono text-base-content/30 shrink-0">{report.ticket_number}</span>
+ <div class="badge badge-sm {priorityConfig[getPriorityKey(report.priority)].color}">
+ {priorityConfig[getPriorityKey(report.priority)].label}
+ </div>
+ </div>
+ </div>
+ </div>
{/each}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/components/ui/tickets/TicketBoard.svelte` around lines 154 -
254, The two role branches are identical; remove the `{`#if` role === "student"}
... {:else if role === "admin"} ... {/if}` split and replace with a single
role-agnostic card block that uses the same markup and handlers. Keep the
existing attributes and handlers (onclick={() => navigate(report)}, onkeydown
handling Enter/Space), and retain usage of report, statusConfig[report.status],
priorityConfig[getPriorityKey(report.priority)], and ticket_number; ensure
navigate(report), getPriorityKey, statusConfig and priorityConfig remain
referenced exactly as before. This collapses duplication while preserving
behavior that varies only inside navigate().
Comment on lines
+16
to
+21
| unsubscribeAuth = authStore.subscribe(($auth) => { | ||
| if ($auth.isAuthenticated) { | ||
| wsStore.connect(); | ||
| } else { | ||
| wsStore.disconnect(); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd "websocket.store.ts" --exec cat {}Repository: Polqt/iComplain
Length of output: 5140
wsStore.connect() lacks guard for CONNECTING state, allowing duplicate WebSocket creation.
The current implementation only guards against the OPEN state (line 19-22), but creates a new WebSocket if called while a previous connection attempt is still in progress. During auth state changes, this can spawn multiple competing WebSocket instances.
Add a check for WebSocket.CONNECTING state alongside the OPEN check:
Suggested guard enhancement
function connect() {
if (socket?.readyState === WebSocket.OPEN || socket?.readyState === WebSocket.CONNECTING) {
console.log("WebSocket already connected or connecting");
return;
}
// ... rest of implementation
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/routes/`+layout.svelte around lines 16 - 21, The connect logic
in wsStore can create duplicate sockets because it only checks for
WebSocket.OPEN; update the connect function (wsStore.connect) to also guard
against WebSocket.CONNECTING by checking socket?.readyState ===
WebSocket.CONNECTING (in addition to OPEN) and early-return with a log/message
if true, so a pending connection attempt won't spawn another socket; ensure this
check is placed before any new WebSocket(...) creation and that
disconnect/connect callers (like the subscription in +layout.svelte) can rely on
this guard.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…age group subscriptions accordingly
…Polqt/iComplain into feat/student-ticket-attachments
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Contributor
Reviewer's GuideImplements a real-time ticketing experience with role-aware layouts by wiring Django Channels-based WebSockets into Svelte stores and layouts, centralizing notification state, fixing WebSocket payload shapes for comments, and adding admin ticket attachment viewing and comment integration. Sequence diagram for comment creation and real-time updatessequenceDiagram
actor Student
participant Browser
participant CommentSection
participant commentsStore
participant DjangoAPI
participant TicketNotificationConsumer
participant wsStore
participant ticketsStore
Student->>Browser: Submit comment form
Browser->>CommentSection: onSubmit
CommentSection->>commentsStore: createComment(ticketId, payload, attachment)
commentsStore->>DjangoAPI: POST /tickets/{id}/comments
DjangoAPI-->>commentsStore: 201 Comment
commentsStore->>commentsStore: addCommentToStore(newComment) (dedupe)
commentsStore-->>CommentSection: newComment
DjangoAPI->>TicketNotificationConsumer: channel_layer.group_send(comment_updates)
TicketNotificationConsumer-->>wsStore: WebSocket message type=comment_created
wsStore->>wsStore: handleMessage(event)
wsStore->>commentsStore: addCommentToStore(comment)
wsStore->>ticketsStore: adjustCommentCount(ticketId, +1)
commentsStore-->>CommentSection: updated comments
ticketsStore-->>Browser: updated tickets list
Class diagram for updated stores and WebSocket typesclassDiagram
class TicketsState {
Ticket[] tickets
boolean isLoading
string error
string currentView
}
class Ticket {
number id
string ticket_number
string title
string description
string status
number priority
string attachment
string created_at
string updated_at
number comments_count
}
class TicketsStore {
+subscribe(run)
+setTickets(tickets)
+setLoading(isLoading)
+setError(error)
+loadTickets()
+loadCommunityTickets()
+reloadTickets()
+createTicket(payload, attachment)
+deleteTicket(id)
+adminPatchTicket(id, patch)
+addTicketToStore(ticket)
+updateTicketInStore(id, updates)
+removeTicketFromStore(id)
+adjustCommentCount(ticketId, delta)
}
class TicketComment {
number id
number ticket
User user
string message
string created_at
}
class CommentsState {
TicketComment[] comments
boolean isLoading
string error
}
class CommentsStore {
+subscribe(run)
+loadComments(ticketId)
+createComment(ticketId, payload, attachment)
+updateComment(ticketId, commentId, payload)
+deleteComment(ticketId, commentId)
+addCommentToStore(comment)
+updateCommentInStore(commentId, updates)
+removeCommentFromStore(commentId)
+clear()
}
class Notification {
string id
string title
string message
string timestamp
boolean read
string actionUrl
string actionLabel
}
class NotificationState {
Notification[] notifications
number unreadCount
boolean isLoading
}
class NotificationStore {
+subscribe(run)
+loadNotifications(limit)
+addNotification(notification)
+markAsRead(id)
+markAllAsRead()
+clear()
}
class WSStatus {
<<enumeration>>
connected
disconnected
connecting
}
class WSMessage {
string action
string type
number ticket_id
any comment
string message
any notification
}
class WSStore {
+subscribe(run)
+connect()
+disconnect()
-handleMessage(event)
-scheduleReconnect()
}
TicketsStore --> TicketsState
TicketsStore --> Ticket
CommentsStore --> CommentsState
CommentsStore --> TicketComment
NotificationStore --> NotificationState
NotificationStore --> Notification
WSStore --> WSStatus
WSStore --> WSMessage
TicketsStore ..> CommentsStore : via comment counts
WSStore ..> TicketsStore : updates tickets
WSStore ..> CommentsStore : updates comments
WSStore ..> NotificationStore : pushes notifications
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- TicketBoard.svelte now duplicates the same card markup for student and admin roles; consider keeping a single card template and only branching on navigation behavior or props to reduce maintenance overhead.
- In TicketNotificationConsumer.connect you call group_add for the user group both inside the authenticated branch and again unconditionally afterward; this results in a duplicate group_add and can be simplified by adding the user group only once.
- Notification store currently prepends every new notification indefinitely while loadNotifications uses a limit; you may want to enforce a maximum length (e.g. trim to the same limit) in addNotification to keep the list size bounded and behavior consistent.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- TicketBoard.svelte now duplicates the same card markup for student and admin roles; consider keeping a single card template and only branching on navigation behavior or props to reduce maintenance overhead.
- In TicketNotificationConsumer.connect you call group_add for the user group both inside the authenticated branch and again unconditionally afterward; this results in a duplicate group_add and can be simplified by adding the user group only once.
- Notification store currently prepends every new notification indefinitely while loadNotifications uses a limit; you may want to enforce a maximum length (e.g. trim to the same limit) in addNotification to keep the list size bounded and behavior consistent.
## Individual Comments
### Comment 1
<location path="backend/apps/tickets/consumers.py" line_range="18-22" />
<code_context>
+ await self.channel_layer.group_add("comment_updates", self.channel_name)
+ await self.channel_layer.group_add("feedback_updates", self.channel_name)
+
+ # Join user-specific group for per-user notifications (e.g. status changes, comments)
+ user = self.scope.get("user")
+ if user and user.is_authenticated:
+ self.user_group = f"user_{user.id}"
+ await self.channel_layer.group_add(self.user_group, self.channel_name)
+ else:
+ self.user_group = None
</code_context>
<issue_to_address>
**issue (bug_risk):** `user_group` is added twice and may be `None`, which can break group_add.
In `connect`, `self.user_group` is set and `group_add` is called inside the authenticated-user branch, but then `group_add(self.user_group, ...)` is called again unconditionally. This duplicates the subscription for authenticated users and would raise if `self.user_group` were ever `None`. Since unauthenticated users are already handled earlier in `connect`, you can safely remove the second `group_add` invocation and keep a single call guarded by the auth check.
</issue_to_address>
### Comment 2
<location path="frontend/src/components/layout/StudentLayout.svelte" line_range="50-52" />
<code_context>
async function handleMarkAsRead(event: CustomEvent<{ id: string }>) {
const { id } = event.detail;
markReadError = "";
- const previousNotifications = notifications.map((n) => ({ ...n }));
- const previousUnreadCount = unreadCount;
- notifications = notifications.map((n) =>
- n.id === id ? { ...n, read: true } : n,
- );
- unreadCount = notifications.filter((n) => !n.read).length;
try {
- await apiMarkAsRead(id);
+ await notificationStore.markAsRead(id);
} catch (e) {
- console.error("Failed to mark notification as read", e);
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Error handling for `markAsRead` no longer aligns with the store implementation.
The layout still keeps `markReadError` and the toast UI, but `notificationStore.markAsRead` now catches/logs errors and doesn’t throw. This handler will therefore never receive an error to set `markReadError`, so the toast path is dead code. Either propagate errors from the store (e.g., rethrow or return a result) so the layout can show feedback, or remove the local error state/toast logic to avoid misleading, unused state.
Suggested implementation:
```
$: notifications = $notificationStore.notifications.map((n) => ({
...n,
timestamp: formatNotificationTimestamp(n.timestamp),
}));
$: unreadCount = $notificationStore.unreadCount;
function openModal() {
async function handleMarkAsRead(event: CustomEvent<{ id: string }>) {
const { id } = event.detail;
await notificationStore.markAsRead(id);
window.addEventListener("resize", checkMobile);
checkMobile();
```
The template likely still contains UI for the toast/error message bound to `markReadError` (e.g., `{#if markReadError}{/if}` or similar). That markup should be removed or refactored, since `markReadError` no longer exists and error handling is delegated entirely to `notificationStore.markAsRead`. If you want to preserve user feedback, instead have `notificationStore.markAsRead` return a result or expose an error store that the layout can subscribe to and display.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+18
to
+22
| # Join user-specific group for per-user notifications (e.g. status changes, comments) | ||
| user = self.scope.get("user") | ||
| if user and user.is_authenticated: | ||
| self.user_group = f"user_{user.id}" | ||
| await self.channel_layer.group_add(self.user_group, self.channel_name) |
Contributor
There was a problem hiding this comment.
issue (bug_risk): user_group is added twice and may be None, which can break group_add.
In connect, self.user_group is set and group_add is called inside the authenticated-user branch, but then group_add(self.user_group, ...) is called again unconditionally. This duplicates the subscription for authenticated users and would raise if self.user_group were ever None. Since unauthenticated users are already handled earlier in connect, you can safely remove the second group_add invocation and keep a single call guarded by the auth check.
Comment on lines
50
to
+52
| markReadError = ""; | ||
| const previousNotifications = notifications.map((n) => ({ ...n })); | ||
| const previousUnreadCount = unreadCount; | ||
| notifications = notifications.map((n) => | ||
| n.id === id ? { ...n, read: true } : n, | ||
| ); | ||
| unreadCount = notifications.filter((n) => !n.read).length; | ||
| try { | ||
| await apiMarkAsRead(id); | ||
| await notificationStore.markAsRead(id); |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): Error handling for markAsRead no longer aligns with the store implementation.
The layout still keeps markReadError and the toast UI, but notificationStore.markAsRead now catches/logs errors and doesn’t throw. This handler will therefore never receive an error to set markReadError, so the toast path is dead code. Either propagate errors from the store (e.g., rethrow or return a result) so the layout can show feedback, or remove the local error state/toast logic to avoid misleading, unused state.
Suggested implementation:
$: notifications = $notificationStore.notifications.map((n) => ({
...n,
timestamp: formatNotificationTimestamp(n.timestamp),
}));
$: unreadCount = $notificationStore.unreadCount;
function openModal() {
async function handleMarkAsRead(event: CustomEvent<{ id: string }>) {
const { id } = event.detail;
await notificationStore.markAsRead(id);
window.addEventListener("resize", checkMobile);
checkMobile();
The template likely still contains UI for the toast/error message bound to markReadError (e.g., {#if markReadError}{/if} or similar). That markup should be removed or refactored, since markReadError no longer exists and error handling is delegated entirely to notificationStore.markAsRead. If you want to preserve user feedback, instead have notificationStore.markAsRead return a result or expose an error store that the layout can subscribe to and display.
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.
Summary by Sourcery
Integrate real-time WebSocket-driven ticket, comment, and notification updates across the app, enhance admin ticket handling with attachments and comments, and centralize notification and ticket state management.
New Features:
Bug Fixes:
Enhancements:
Build:
Summary by CodeRabbit
Release Notes
New Features
Refactor