feat: add alarms system with notifications#331
feat: add alarms system with notifications#331maoshuorz wants to merge 1 commit intodatabuddy-analytics:mainfrom
Conversation
|
@maoshuorz is attempting to deploy a commit to the Databuddy OSS Team on Vercel. A member of the Team first needs to authorize it. |
|
|
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
Greptile SummaryThis PR implements a full alarms system allowing users to create notification alarms triggered by uptime, traffic spikes, error rates, goals, or custom conditions — with delivery via Slack, Discord, Email, or custom webhooks. It adds a new The overall structure is solid and follows existing project patterns well. There is one UX issue that should be addressed:
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant UI as Dashboard UI
participant Router as alarms Router
participant AuthFn as authorizeScope
participant DB as Database
participant Notif as notifications pkg
UI->>Router: list alarms for org
Router->>AuthFn: check read permission
AuthFn-->>Router: allowed or forbidden
Router->>DB: select non-deleted alarms
DB-->>Router: alarm rows
Router-->>UI: alarm list
UI->>Router: create alarm
Router->>AuthFn: check write permission
AuthFn-->>Router: allowed or forbidden
Router->>DB: insert new alarm
DB-->>Router: created alarm
Router-->>UI: alarm record
UI->>Router: update alarm
Router->>DB: fetch alarm by id
DB-->>Router: alarm record
Router->>AuthFn: check write permission
AuthFn-->>Router: allowed or forbidden
Router->>DB: update alarm fields
DB-->>Router: updated alarm
Router-->>UI: updated record
UI->>Router: test alarm
Router->>DB: fetch alarm by id
DB-->>Router: alarm record
Router->>AuthFn: check write permission
AuthFn-->>Router: allowed or forbidden
Router->>Notif: send to Slack, Discord, Webhook
Notif-->>Router: per-channel results
Router-->>UI: overall success plus results (email always fails)
UI->>Router: delete alarm
Router->>DB: fetch alarm scope
DB-->>Router: websiteId or orgId
Router->>AuthFn: check delete permission
AuthFn-->>Router: allowed or forbidden
Router->>DB: soft delete and disable
DB-->>Router: done
Router-->>UI: success true
Last reviewed commit: 49f4aba |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| case "email": { | ||
| // Email sending requires an email provider configured at app level | ||
| results.push({ | ||
| channel: "email", | ||
| success: false, | ||
| error: | ||
| "Email notifications require server-side email configuration", | ||
| }); | ||
| break; | ||
| } |
There was a problem hiding this comment.
The email case unconditionally returns success: false, which causes misleading UX when email is the only configured channel. Since the overall success is computed as results.some((r) => r.success) (line 497), a user with a valid email-only alarm configuration will always see "Test notification failed" in the UI.
Consider either:
- Excluding email from the notification channel options (mark as "coming soon")
- Returning a distinct non-failure status or surfacing a different message like "Email test not yet supported"
- Implementing server-side email functionality so the test can actually complete
| const addEmail = () => { | ||
| const trimmed = emailInput.trim(); | ||
| if (!trimmed) { | ||
| return; | ||
| } | ||
| const current = form.getValues("emailAddresses") || []; | ||
| if (!current.includes(trimmed)) { | ||
| form.setValue("emailAddresses", [...current, trimmed]); | ||
| } | ||
| setEmailInput(""); | ||
| }; |
There was a problem hiding this comment.
The addEmail() function accepts any non-empty string without validating email format before adding to the form field. Invalid entries like "not-an-email" appear as badges in the UI and are only rejected at submit time by Zod validation.
Consider adding a quick client-side format check before calling form.setValue for faster feedback:
| const addEmail = () => { | |
| const trimmed = emailInput.trim(); | |
| if (!trimmed) { | |
| return; | |
| } | |
| const current = form.getValues("emailAddresses") || []; | |
| if (!current.includes(trimmed)) { | |
| form.setValue("emailAddresses", [...current, trimmed]); | |
| } | |
| setEmailInput(""); | |
| }; | |
| const addEmail = () => { | |
| const trimmed = emailInput.trim(); | |
| if (!trimmed || !/^[^\s@]+@[^\s@]+\.[^\s@]+$/.test(trimmed)) { | |
| return; | |
| } | |
| const current = form.getValues("emailAddresses") || []; | |
| if (!current.includes(trimmed)) { | |
| form.setValue("emailAddresses", [...current, trimmed]); | |
| } | |
| setEmailInput(""); | |
| }; |
|
recheck |
|
Closing as part of maintainer triage: we’re not merging unsolicited / AI-style bulk bounty PRs for this area. If you plan a good-faith contribution later, open a new PR with a short description that matches the actual diff. |
3 participants
Summary
Resolves #267
Adds a full alarms system for monitoring website events and sending notifications via Slack, Discord, Email, or custom Webhooks.
Changes
Database
alarmstable with fields for name, description, enabled state, notification channels, webhook URLs, trigger type/conditionsAPI (ORPC Router)
Shared Validation (
@databuddy/shared/alarms)alarmFormSchema,createAlarmSchema,updateAlarmSchema,listAlarmsSchema,getAlarmSchema,deleteAlarmSchema,testAlarmSchemanotificationChannelSchema(slack, discord, email, webhook),triggerTypeSchema(uptime, traffic_spike, error_rate, goal, custom)Dashboard UI
Design
roundedclass (matches existing patterns)anytypes — usesunknownfor JSONB fieldsconsole.logstatements/claim #267