Skip to content

ui notifications#1622

Open
Rainhunter13 wants to merge 2 commits intodevfrom
agent/lam-1417/notifications-in-ui/d32ee5
Open

ui notifications#1622
Rainhunter13 wants to merge 2 commits intodevfrom
agent/lam-1417/notifications-in-ui/d32ee5

Conversation

@Rainhunter13
Copy link
Copy Markdown
Member

@Rainhunter13 Rainhunter13 commented Apr 9, 2026

Note

Medium Risk
Introduces a new notifications API backed by ClickHouse plus a Postgres notification_reads table for per-user read state, which could affect data access and query performance. UI adds client-side SWR fetching and optimistic read updates, so regressions would mostly be around correctness of unread state and workspace/project authorization checks.

Overview
Adds an in-app Notifications experience: a bell trigger in the project sidebar opens a sliding NotificationPanel that lists recent notifications, supports expanding details, deep-links to traces, and shows an unread indicator.

Implements a new /api/workspaces/[workspaceId]/notifications route with session auth and workspace/project validation, plus backend actions to fetch recent notifications from ClickHouse and persist per-user read status in a new Postgres notification_reads table (with a migration and Drizzle schema).

Reviewed by Cursor Bugbot for commit 8a30e8b. Bugbot is set up for automated code reviews on this repo. Configure here.

Copy link
Copy Markdown
Contributor

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit a9c45ad. Configure here.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Apr 9, 2026

Greptile Summary

This PR adds an in-app notifications experience: a bell-icon trigger in the project sidebar opens a slide-out NotificationPanel that lists recent signal-event reports fetched from ClickHouse, supports expand/collapse of details, and deep-links into relevant traces. Read state is persisted per-user in a new Postgres table (notification_reads) with optimistic UI updates.

  • New GET/POST /api/workspaces/[workspaceId]/notifications route backed by getWebNotifications and markNotificationAsRead server actions
  • New notification_reads Postgres table (migration + Drizzle schema) with composite PK on (project_id, user_id, notification_id) and cascade-delete FKs
  • New isProjectInWorkspace authorization helper used by both route handlers
  • NotificationTrigger and NotificationPanel share the same SWR key so they use a single cached fetch with no duplicate network requests
  • formatNotification filters out weekly-style rollups (period > 3 days) to avoid redundancy with daily reports
  • The missing user-membership check on the API route was flagged in a prior review thread; the one new finding is that the handleMarkAsRead error path silently reverts the optimistic update without surfacing a toast to the user, which violates the project's client-side error-handling convention

Confidence Score: 4/5

Mostly safe to merge; the missing user-membership check on the API route (already flagged in prior thread) is the only meaningful risk, and the new inline finding (missing toast) is non-blocking.

The implementation is clean end-to-end — migration, schema, actions, API route, store, and UI components are all consistent with each other and with codebase conventions. The security concern about missing membership checks was already surfaced in a previous review thread. The sole new P1 finding (no toast on mark-as-read failure) is a UX/convention gap rather than a correctness or data-loss issue, keeping the score at 4 rather than 5.

frontend/app/api/workspaces/[workspaceId]/notifications/route.ts (membership authorization), frontend/components/notifications/notification-panel.tsx (error toast)

Vulnerabilities

  • The GET and POST route handlers verify that the project belongs to the workspace (isProjectInWorkspace) but do not verify that the authenticated user is a member of that workspace or project. This was raised in a prior review thread. Until a membership check (e.g. isUserMemberOfProject) is added, any authenticated user who can guess a valid workspaceId+projectId pair can read notification payloads or write read-state records for that project.

Important Files Changed

Filename Overview
frontend/app/api/workspaces/[workspaceId]/notifications/route.ts New GET/POST notification endpoints; guarded by workspace/project check but missing user-membership verification (flagged in prior review thread).
frontend/components/notifications/notification-panel.tsx Slide-out notification panel with expand/collapse and optimistic read-state updates; silent failure on mark-as-read errors violates project error-handling convention.
frontend/components/notifications/notification-store.ts Minimal Zustand store for panel open/close state; functional-updater pattern is safe for concurrent calls.
frontend/components/notifications/notification-trigger.tsx Bell-icon trigger button with unread badge; shares SWR cache with the panel via identical key — no duplicate network requests.
frontend/lib/actions/notifications/index.ts ClickHouse query + Postgres read-state join logic looks correct; notificationId is accepted as any valid UUID without verifying it exists in CH for the given project (data-integrity edge case, not a security breach).
frontend/lib/authorization/index.ts New isProjectInWorkspace helper — clean addition, correctly limits to 1 row.
frontend/lib/db/migrations/0080_third_cammi.sql Creates notification_reads table with composite PK and cascade-delete FK constraints — looks correct.
frontend/lib/db/migrations/schema.ts Drizzle schema entry for notification_reads matches the SQL migration exactly.
frontend/app/project/[projectId]/layout.tsx Adds NotificationPanel inside SidebarInset; relative-positioned parent provides correct anchor for absolute-positioned panel.
frontend/components/project/sidebar/header.tsx Adds NotificationTrigger to expanded sidebar header — straightforward integration.

Reviews (2): Last reviewed commit: "fix: use trace_id instead of array index..." | Re-trigger Greptile

@Rainhunter13
Copy link
Copy Markdown
Member Author

@laminar-coding-agent

validate and address open pr comments

@Rainhunter13
Copy link
Copy Markdown
Member Author

@laminar-coding-agent /autoreply start

…rthy events

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@CLAassistant
Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ Rainhunter13
❌ greptile-apps[bot]
You have signed the CLA already but the status is still pending? Let us recheck it.

@laminar-coding-agent
Copy link
Copy Markdown
Contributor

Here's a summary of the 4 PR review comments and their dispositions:

1. Missing user membership check (Cursor bot + Greptile — both P0/High)False positive. proxy.ts middleware already calls isUserMemberOfWorkspace(workspaceId, userId) for all /api/workspaces/:workspaceId/... routes before the handler runs. The isProjectInWorkspace check in the route handler adds the second layer (ensuring the project belongs to the workspace). No additional auth check needed.

2. Array index used as React list key (Greptile — P2)Valid. Fixed by replacing key={i} with key={event.trace_id} on the noteworthy events list. Each NoteworthyEvent has a trace_id field that serves as a stable identifier.

3. Zustand store race condition documentation (Greptile — P2)False positive / unnecessary. The store already uses the functional updater pattern (set((state) => ...)) which is safe, and exposes both toggle and close. Adding a comment explaining this would be over-engineering for a 4-line store.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants