feat: notify staff on task complete#72
Conversation
WalkthroughAdds a notifications feature: UI (header bell, unread indicator, drawer), app state to open/close it, store/type refactors, server-side creation of task-completed notifications for staff, repository and DB schema changes, and updates notification polling/refresh timing. Changes
Sequence Diagram(s)sequenceDiagram
actor Staff as Staff User
participant UI as Web App UI
participant API as Server API
participant DB as Database
Staff->>UI: Complete task
UI->>API: POST /api/task/:id/complete
API->>DB: Update task status
API->>DB: Query staff users (exclude actor)
loop per recipient
API->>DB: Insert notification(authorId, userId, taskId, title, description)
end
API-->>UI: 200 OK
sequenceDiagram
participant Header as Header Bell
participant App as useApp()
participant Drawer as NotificationsDrawer
participant Store as NotificationStore
Header->>App: set isNotificationsOpened = true
App-->>Drawer: isNotificationsOpened (ref)
Drawer->>Store: read notifications[]
Drawer->>Drawer: render list (author, title, desc, time)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (16)
apps/web-app/app/components/HeaderMenuButton.vue (1)
2-8: Add an accessible label for the icon-only buttonIcon-only buttons need an aria-label for screen readers. Consider adding a label (and optionally reflect expanded state if applicable).
Apply this diff:
<UButton icon="i-lucide-menu" color="neutral" variant="outline" square + aria-label="Open menu" @click="isNavbarOpened = true" />apps/web-app/app/composables/useApp.ts (1)
8-14: DRY up overlay closing with a helperMinor: centralize closing logic (navbar + notifications) to avoid duplicating assignments and to enable reuse elsewhere (e.g., ESC handler).
watch( () => route.fullPath, - () => { - isNavbarOpened.value = false - isNotificationsOpened.value = false - }, + () => closeOverlays(), ) return { isNavbarOpened, isNotificationsOpened, imagesMode, + closeOverlays, } } export const useApp = createSharedComposable(_useApp) + +function closeOverlays() { + isNavbarOpened.value = false + isNotificationsOpened.value = false +}Also applies to: 16-21
packages/database/src/tables.ts (1)
367-387: Add indexes for frequent queries (list by user ordered by createdAt)Your repository queries notifications by
userIdordered bycreatedAt. Adding a composite index improves latency at scale.Add the extra config block:
// Place right under the notifications pgTable definition import { index } from 'drizzle-orm/pg-core' export const notifications = pgTable('notifications', { // columns... }, (table) => { return { userCreatedAtIdx: index('notifications_user_id_created_at_idx').on(table.userId, table.createdAt), } })apps/web-app/app/stores/notification.ts (3)
17-25: Strongly type the fetch for safer assignmentsType the fetch result to ensure
notifications.valuestays consistent and catch backend/typing drift at compile time.- const data = await $fetch('/api/notification/my') + const data = await $fetch<NotificationWithEntities[]>('/api/notification/my')
37-69: Consider enriching the toast contentOptional: Use the author as the avatar fallback (if performer is absent) and set a meaningful alt. Also, your second action label is hard-coded to "0".
toastContext.add({ id: notification.id, title: notification.title, description: notification.description ?? '', avatar: { - src: notification.task?.performer?.avatarUrl ?? undefined, - alt: '', + src: notification.task?.performer?.avatarUrl ?? notification.author.avatarUrl ?? undefined, + alt: notification.author.name || '', }, color: 'info', duration: 60000, actions: [{ icon: 'i-lucide-thumbs-up', label: 'Лайк', color: 'neutral', variant: 'outline', size: 'md', onClick: (e) => { e?.stopPropagation() }, }, { icon: 'fluent-emoji-flat:party-popper', - label: '0', + label: '0', // TODO: replace with dynamic metric if/when available color: 'info', variant: 'soft', size: 'md', ui: { label: 'font-semibold', leadingIcon: 'motion-preset-pulse motion-duration-1500', }, disabled: true, }], })
98-109: Stale watcher block and outdated function nameThe watcher is commented out and references
showCompletedTaskToastwhich no longer exists. Either remove it or update the reference if you intend to re-enable.Option A — remove the commented block (cleanest):
- // watch(notifications, () => { - // for (const notification of notifications.value) { - // // already shown? - // if (toastContext.toasts.value.find((toast) => toast.id === notification.id)) { - // continue - // } - // - // if (notification.type === 'task_completed') { - // showCompletedTaskToast(notification) - // } - // } - // }) + // Intentionally left disabled: toast auto-display on updates.Option B — fix the function name if you plan to re-enable:
- // showCompletedTaskToast(notification) + // _showCompletedTaskToast(notification)apps/web-app/app/app.vue (1)
68-92: Avoid overlapping polling cycles; fix interval typing for SSR/CSR
setIntervaldoesn’t await your async work; if requests exceed 30s, cycles overlap. Also,NodeJS.Timeoutcan be incorrect in browsers. Use a reentrancy guard and a portable interval type.-// Auto Update Online -let interval: NodeJS.Timeout +// Auto Update Online +let interval: ReturnType<typeof setInterval> +let isRefreshing = false + +async function refreshAll() { + if (isRefreshing) return + isRefreshing = true + try { + await Promise.all([ + user.updateOnline(), + user.update(), + task.update(), + ticket.update(), + notification.update(), + ]) + } finally { + isRefreshing = false + } +} onMounted(async () => { - await Promise.all([ - user.updateOnline(), - user.update(), - task.update(), - ticket.update(), - notification.update(), - ]) + await refreshAll() interval = setInterval(async () => { - await Promise.all([ - user.updateOnline(), - user.update(), - task.update(), - ticket.update(), - notification.update(), - ]) + await refreshAll() }, 30000) }) onUnmounted(() => { clearInterval(interval) })packages/database/src/repository/notification.ts (3)
16-18: Add an index to support where(userId)=… and order by createdAt descThe query now filters by userId, sorts by createdAt desc, and limits 250. To keep it fast at scale, add an index compatible with this access pattern (e.g., composite on (user_id, created_at desc) or (user_id, created_at)). If you already have one, ignore.
Example migration (pseudo):
CREATE INDEX IF NOT EXISTS idx_notifications_user_created_at ON notifications (user_id, created_at DESC);
19-25: Trim related columns to reduce payload and PII surfacewith: { author: true, task: { with: { performer: true } } } will hydrate full user/task/performer models. If the UI needs only a subset (e.g., id, name, surname, avatarUrl), select those columns to reduce response size and exposure.
Example (Drizzle style):
return useDatabase().query.notifications.findMany({ where: (n, { eq }) => eq(n.userId, userId), orderBy: (n, { desc }) => desc(n.createdAt), limit: 250, with: { author: { columns: { id: true, name: true, surname: true, avatarUrl: true }, }, task: { columns: { id: true, name: true }, with: { performer: { columns: { id: true, name: true, surname: true, avatarUrl: true }, }, }, }, }, })
16-18: Consider making the limit configurable and centralizedHardcoding 250 is fine for now, but a constant or parameter improves reuse and testing.
- limit: 250, + limit: MAX_NOTIFICATIONS,Additionally:
// packages/database/src/repository/constants.ts export const MAX_NOTIFICATIONS = 250apps/web-app/server/api/task/id/[taskId]/complete.post.ts (2)
99-101: Avoid loading all users; fetch only active staff server-sideLoading all users and filtering in memory won’t scale. Prefer a repository method or query that returns only active staff (and excludes the actor) to minimize data and DB load.
For example, introduce repository.user.listActiveStaffExcept(userId) and use it here:
const recipients = await repository.user.listActiveStaffExcept(user.id)Interim in-file improvement (still not ideal DB-wise):
-const users = await repository.user.list() -const allStaffExceptUser = users.filter((u) => u.type === 'staff' && u.id !== user.id) +const users = await repository.user.list() +const allStaffExceptUser = users.filter((u) => u.type === 'staff' && u.isActive && u.id !== user.id)
102-111: Create notifications concurrently to reduce latencyCreating notifications one-by-one increases request time for large recipient sets. Batching or concurrency helps. Given current APIs, Promise.all is a simple win; for even better performance, consider a repository.notification.createMany to insert in bulk.
Apply this diff:
-for (const staff of allStaffExceptUser) { - await repository.notification.create({ - authorId: user.id, - userId: staff.id, - taskId: updatedTask.id, - type: 'task_completed', - title: `${suffixByGender(['Завершил', 'Завершила'], user.gender)} задачу «${updatedTask.name}»`, - description: updatedTask.report ? updatedTask.report : 'Без отчета', - }) -} +await Promise.all( + allStaffExceptUser.map((staff) => + repository.notification.create({ + authorId: user.id, + userId: staff.id, + taskId: updatedTask.id, + type: 'task_completed', + title: `${suffixByGender(['Завершил', 'Завершила'], user.gender)} задачу «${updatedTask.name}»`, + description: updatedTask.report ? updatedTask.report : 'Без отчета', + }), + ), +)If you’re concerned about connection saturation, chunk the array into small batches (e.g., size 10) and await each batch.
apps/web-app/app/components/NotificationsDrawer.vue (2)
41-43: Guard nullable descriptions to avoid rendering “null”description is nullable in the schema. Rendering it directly may show “null”. Guard it or provide a fallback.
Apply this diff:
-<p class="text-sm text-dimmed"> - {{ notification.description }} -</p> +<p v-if="notification.description" class="text-sm text-dimmed"> + {{ notification.description }} +</p>
17-21: Accessibility: add avatar alt textIf UAvatar supports an alt prop, pass the author’s name to improve accessibility.
Example:
-<UAvatar - :src="notification.author.avatarUrl ?? undefined" - size="lg" -/> +<UAvatar + :src="notification.author.avatarUrl ?? undefined" + :alt="`${notification.author.name} ${notification.author.surname}`" + size="lg" />apps/web-app/app/components/Header.vue (2)
14-29: Hook up the “N” shortcut to actually open the drawerTooltip advertises a shortcut, but there’s no listener here. If there isn’t a global shortcut system elsewhere, attach a key listener.
You can add a simple listener:
<script setup lang="ts"> defineProps<{ title: string }>() const { isNotificationsOpened } = useApp() const notificationStore = useNotificationStore() const haveNotifications = computed(() => notificationStore.notifications.filter((notification) => !notification.viewedAt).length > 0) + +onMounted(() => { + const handler = (e: KeyboardEvent) => { + if ((e.key === 'n' || e.key === 'N') && !e.metaKey && !e.ctrlKey && !e.altKey) { + isNotificationsOpened.value = true + } + } + window.addEventListener('keydown', handler) + onBeforeUnmount(() => window.removeEventListener('keydown', handler)) +}) </script>If you already have a global shortcuts plugin, ignore and keep the tooltip label only.
47-49: Micro-optimization: use some() for unread checksome() short-circuits on first unread item and reads cleaner.
Apply this diff:
-const haveNotifications = computed(() => notificationStore.notifications.filter((notification) => !notification.viewedAt).length > 0) +const haveNotifications = computed(() => notificationStore.notifications.some((n) => !n.viewedAt))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (10)
apps/web-app/app/app.vue(2 hunks)apps/web-app/app/components/Header.vue(2 hunks)apps/web-app/app/components/HeaderMenuButton.vue(1 hunks)apps/web-app/app/components/NotificationsDrawer.vue(1 hunks)apps/web-app/app/composables/useApp.ts(1 hunks)apps/web-app/app/layouts/default.vue(1 hunks)apps/web-app/app/stores/notification.ts(3 hunks)apps/web-app/server/api/task/id/[taskId]/complete.post.ts(1 hunks)packages/database/src/repository/notification.ts(3 hunks)packages/database/src/tables.ts(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (5)
apps/web-app/app/app.vue (4)
apps/web-app/app/stores/ticket.ts (1)
update(12-30)apps/web-app/app/stores/activity.ts (2)
update(10-25)schedules(7-32)apps/web-app/app/stores/user.ts (1)
update(25-54)apps/web-app/app/stores/partner.ts (1)
update(28-48)
apps/web-app/app/composables/useApp.ts (1)
apps/web-app/app/composables/useTicket.ts (2)
_useTicket(1-31)route(24-24)
packages/database/src/repository/notification.ts (4)
packages/database/src/tables.ts (1)
notifications(367-387)packages/database/src/repository/activity.ts (2)
Activity(6-71)updateSchedule(40-50)packages/database/src/repository/user.ts (1)
update(80-90)packages/database/src/repository/ticket.ts (1)
update(58-68)
apps/web-app/server/api/task/id/[taskId]/complete.post.ts (3)
packages/database/src/tables.ts (1)
users(83-100)packages/database/src/repository/index.ts (1)
repository(57-57)apps/web-app/shared/utils/gender.ts (1)
suffixByGender(3-11)
apps/web-app/app/stores/notification.ts (2)
packages/database/src/types.ts (3)
Task(69-69)User(9-9)Notification(78-78)packages/database/src/repository/notification.ts (1)
Notification(6-49)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (15)
apps/web-app/app/composables/useApp.ts (1)
5-13: Good addition: notifications drawer state + route-change resetAdding
isNotificationsOpenedand resetting it on route changes mirrors the navbar behavior and prevents sticky drawers across navigations. Looks consistent with the new UI.packages/database/src/tables.ts (5)
371-371: Adding viewedAt enables unread state — LGTMNullable
viewedAtis a clean way to model unread notifications and aligns with the UI.
374-374: Make description nullable — LGTMThis unblocks notifications without descriptions and matches the frontend’s null-safe usage.
867-869: Add inverse relation tasks -> notifications — LGTMThis simplifies loading notifications for a task and matches the FK on notifications.taskId.
888-891: Add author relation — LGTMAligns with UI requirements to show the author. Matches repository
.with({ author: true }).
375-378: Reconsider authorId constraints and ensure safe migration/backfillPlease address the following before merging:
• Add or verify a migration that adds
author_idtonotificationswith a safe backfill for existing rows (e.g. populate from audit log or default) so that theNOT NULLconstraint won’t fail at deploy.
• Review all schema migration scripts (SQL or TS) and confirm they include the backfill and constraint change.
• Decide on the desired deletion behavior and update the foreign‐key accordingly:
–onDelete: 'restrict'to prevent deleting authors with notifications (preserve history), or
– makeauthorIdnullable withonDelete: 'set null'if you must allow user deletions and preserve notifications.
• Audit everyinsert(notifications).values({…})call in the codebase to ensureauthorIdis always provided.Suggested schema update (pick one):
- authorId: cuid2('author_id').notNull().references(() => users.id, { - onDelete: 'cascade', - onUpdate: 'cascade', - }), + // Option A: Preserve history by preventing author deletion + authorId: cuid2('author_id').notNull().references(() => users.id, { + onDelete: 'restrict', + onUpdate: 'cascade', + }), + + // Option B: Allow author deletion and keep notifications + // authorId: cuid2('author_id').references(() => users.id, { + // onDelete: 'set null', + // onUpdate: 'cascade', + // }),apps/web-app/app/stores/notification.ts (2)
3-10: Types aligned with backend shape — LGTMIntroducing TaskWithPerformer and adding author to NotificationWithEntities matches repository
.with({ author, task: { performer } }).
41-44: Null-safe UI data access — LGTMUsing
?? ''for description and?.for avatar prevents runtime errors on incomplete entities.apps/web-app/app/app.vue (1)
55-66: Intentional move of notification update to onMounted — confirm SSR expectationsYou moved
notification.update()out of the initial pre-mount batch and into onMounted/interval. This avoids SSR-time fetches but means the drawer starts empty until mount. If that’s intentional, all good; otherwise consider adding it back to the initial Promise.all to hydrate earlier.Also applies to: 70-78
apps/web-app/app/layouts/default.vue (1)
8-9: LGTM: Notifications drawer inclusion is well-placedRendering the NotificationsDrawer at the layout root (as a sibling to USlideover) is appropriate for overlays. No issues spotted here.
packages/database/src/repository/notification.ts (2)
2-2: Consistent use of sql tag importBringing in sql from drizzle-orm aligns this repo with other repositories’ update patterns.
37-40: Good: updatedAt is maintained on updateMerging data with updatedAt: sql
now()mirrors other repos and keeps timestamps accurate.apps/web-app/server/api/task/id/[taskId]/complete.post.ts (1)
97-113: Confirm intended trigger: notify staff only when staff completes, or always notify staff?Current guard only notifies when user.type === 'staff'. If tasks can be completed by non-staff (e.g., vendors/clients) and staff should still be notified, adjust the condition.
Would you like me to patch the condition and tests if the intent is “notify staff on any task completion”?
apps/web-app/app/components/NotificationsDrawer.vue (1)
28-33: Timezone/SRR hydration sanity checkformat(new Date(notification.createdAt), ...) runs at render time; ensure createdAt includes timezone (it does per schema) to avoid hydration drift. No change required if you’ve verified.
If hydration mismatches are observed, we can render the time client-only or normalize via UTC.
apps/web-app/app/components/Header.vue (1)
14-29: Nice addition: bell with unread indicator and tooltipThe UX is clear and consistent with the new drawer. Opening via isNotificationsOpened is straightforward.
| // Notify all staff | ||
| if (user.type === 'staff') { | ||
| const users = await repository.user.list() | ||
| const allStaffExceptUser = users.filter((u) => u.type === 'staff' && u.id !== user.id) | ||
|
|
||
| for (const staff of allStaffExceptUser) { | ||
| await repository.notification.create({ | ||
| authorId: user.id, | ||
| userId: staff.id, | ||
| taskId: updatedTask.id, | ||
| type: 'task_completed', | ||
| title: `${suffixByGender(['Завершил', 'Завершила'], user.gender)} задачу «${updatedTask.name}»`, | ||
| description: updatedTask.report ? updatedTask.report : 'Без отчета', | ||
| }) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Missing import: suffixByGender is used but not imported (will throw ReferenceError)
suffixByGender is referenced in the title construction here and within prepareBotMessage, but there’s no import in this module.
Apply this diff to the import block:
import { getLocalizedResolution } from '~~/shared/utils/helpers'
+import { suffixByGender } from '~~/shared/utils/gender'🤖 Prompt for AI Agents
In apps/web-app/server/api/task/id/[taskId]/complete.post.ts around lines
97-113, suffixByGender is used but not imported; add the appropriate import for
suffixByGender at the top import block (use the module where suffixByGender is
defined and match named/default export), then run typecheck/build to confirm the
import path and export style are correct so the ReferenceError is resolved.
|
1 participant
Summary by CodeRabbit