feat: Notifications#161
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a backend notifications feature backed by new DB tables and a Shoutrrr-based provider layer, exposing CRUD + test endpoints under /api/v1/notifications and adding unit/integration-style tests.
Changes:
- Introduces
notifications+application_notificationstables and GORM models/associations. - Adds REST endpoints to create/list/update/delete notifications and to send a test notification, with SSE update publishing.
- Adds a notifications package with a provider registry (Discord implementation) and config parsing helpers, plus tests; updates Go deps to include Shoutrrr.
Reviewed changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| backend/internal/hub/routes/notifications.go | New notifications CRUD + test handlers, request/response mapping, config normalization, app association loading |
| backend/internal/hub/routes/notifications_test.go | Handler tests for list/create/update/delete/test endpoints |
| backend/internal/hub/notifications/provider/provider.go | Provider interface + registry (Discord registered in init) |
| backend/internal/hub/notifications/provider/discord.go | Discord config parsing and Shoutrrr URL construction (incl thread_id, username, avatar, title) |
| backend/internal/hub/notifications/provider/common.go | Shared parsing for direct target configs and normalization/deduplication |
| backend/internal/hub/notifications/provider.go | Public wrapper functions (Register/Get/Build*), keeps legacy BuildShoutrrrURLs |
| backend/internal/hub/notifications/notifications.go | Notification dispatch + DB query selection logic + test-send helper |
| backend/internal/hub/notifications/notifications_test.go | Tests for DB query filtering and config parsing/building |
| backend/internal/hub/models/notifications.go | New Notification model and enums |
| backend/internal/hub/models/applications.go | Adds many-to-many Notifications association on Application |
| backend/internal/hub/handlers.go | Registers new protected notification routes |
| backend/internal/hub/db/migrations/000016_create_notifications_tables.up.sql | Creates notifications + join table + indexes |
| backend/internal/hub/db/migrations/000016_create_notifications_tables.down.sql | Drops notifications tables/indexes |
| backend/internal/hub/db/db_test.go | Extends migration tests to assert new tables/columns exist |
| backend/go.mod | Adds Shoutrrr dependency (and related indirect deps updates) |
| backend/go.sum | Dependency checksum updates for new/updated modules |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
| EnableByDefault *bool `json:"enableByDefault"` | ||
| Status models.NotificationStatus `json:"status" binding:"required"` | ||
| Type models.NotificationType `json:"type" binding:"required"` | ||
| Config json.RawMessage `json:"config" binding:"required"` |
There was a problem hiding this comment.
It might be possible to simplify the logic a lot by always using json for the config and always decode it just once and not in route handler and providers?
There was a problem hiding this comment.
Im not sure what you mean by this
There was a problem hiding this comment.
I think there are multiple places in the code where it checks if it's json and also allows non json? E.g. in parseDirectTargets in common.go?
There was a problem hiding this comment.
I tried improving it. Only the providers are parsing now which is necessary. Good enough?
There was a problem hiding this comment.
The modal is a bit large, long-term we can maybe do two steps or collapse additional optional settings, although it's also not nice to hide them.
There was a problem hiding this comment.
The avatar and bot name can also be configured in Discord UI (with image upload). Not sure which of both settings has a higher priority? Maybe enough to just let the user configure them in Discord?
| return ""; | ||
| } | ||
|
|
||
| export default function UpsertNotificationDialog({ |
There was a problem hiding this comment.
In edit mode the dialog closes itself directly when the cursor moves away from the dropdown (at least in Firefox)
| return nil, err | ||
| } | ||
|
|
||
| if strings.HasPrefix(trimmed, "{") { |
There was a problem hiding this comment.
We always send JSON from frontend to backend, or?
Type of change
Description
Fixes #112
TODO: