Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a first version of durable, unread dashboard notifications end-to-end: lifecycle emits notification intents, the backend persists/dedupes and exposes them via a new REST endpoint, and the OpenAPI/TS client types are generated accordingly.
Changes:
- Add notification domain model, SQLite table/migration, sqlc queries, and a notification service with unread dedupe + listing.
- Emit notification intents from lifecycle for
needs_inputand SCM PR state transitions (ready_to_merge,pr_merged,pr_closed_unmerged) and wire the sink in the daemon. - Expose
GET /api/v1/notificationsand update OpenAPI + generated frontend schema/types.
Reviewed changes
Copilot reviewed 26 out of 28 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/api/schema.ts | Adds generated TS types for /api/v1/notifications and notification DTOs. |
| backend/sqlc.yaml | Adds sqlc type overrides for notifications columns to domain types. |
| backend/internal/storage/sqlite/store/notification_store.go | Implements notification persistence + unread listing + dedupe behavior. |
| backend/internal/storage/sqlite/store/notification_store_test.go | Adds store tests for insert/list/dedupe and constraints. |
| backend/internal/storage/sqlite/queries/notifications.sql | Defines sqlc queries for create + unread list (global/by project). |
| backend/internal/storage/sqlite/migrations/0011_notifications.sql | Introduces notifications table, indexes, and unread-dedupe unique index. |
| backend/internal/storage/sqlite/gen/notifications.sql.go | Generated sqlc query code for notifications. |
| backend/internal/storage/sqlite/gen/models.go | Adds generated Notification model. |
| backend/internal/service/notification/types.go | Defines notification service DTOs/filters and intent alias. |
| backend/internal/service/notification/store.go | Declares service store interface used by the manager. |
| backend/internal/service/notification/service.go | Adds notification Manager: enrich, persist (dedupe), dispatch, list. |
| backend/internal/service/notification/service_test.go | Adds Manager tests for persist/dispatch/duplicate/list-target behavior. |
| backend/internal/service/notification/enrich.go | Implements intent → stored record enrichment (title/body). |
| backend/internal/service/notification/dispatcher.go | Adds v1 dispatcher boundary and dashboard dispatcher implementation. |
| backend/internal/ports/notifications.go | Adds lifecycle → notification-service intent contract type. |
| backend/internal/lifecycle/reactions.go | Emits SCM-derived notification intents after successful PR reaction handling. |
| backend/internal/lifecycle/manager.go | Adds optional notification sink, emits needs_input intents on transitions. |
| backend/internal/lifecycle/manager_test.go | Adds tests covering notification emission for activity + SCM observations. |
| backend/internal/httpd/controllers/notifications.go | Adds controller for GET /api/v1/notifications with query parsing + DTO mapping. |
| backend/internal/httpd/controllers/notifications_test.go | Adds controller tests for list, defaults/caps, validation, 501 behavior. |
| backend/internal/httpd/controllers/dto.go | Adds notification request/response DTOs and OpenAPI-tagged query struct. |
| backend/internal/httpd/apispec/specgen/build.go | Registers notification tag + operation and schema name mappings. |
| backend/internal/httpd/apispec/openapi.yaml | Updates committed OpenAPI spec to include notifications endpoint + schemas. |
| backend/internal/httpd/api.go | Wires notifications controller into the API router/deps. |
| backend/internal/domain/notification.go | Adds domain enums and record validation for notifications. |
| backend/internal/daemon/wiring_test.go | Updates lifecycle wiring test for new notifier parameter. |
| backend/internal/daemon/lifecycle_wiring.go | Wires lifecycle Manager with optional notification sink. |
| backend/internal/daemon/daemon.go | Instantiates notification Manager and wires it into lifecycle + HTTP API deps. |
Files not reviewed (2)
- backend/internal/storage/sqlite/gen/models.go: Language not supported
- backend/internal/storage/sqlite/gen/notifications.sql.go: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Greptile SummaryThis PR introduces a durable notification domain for v1: lifecycle events (
Confidence Score: 5/5Safe to merge; the new notification write/read paths are well-isolated, the dedup logic is correct, and the lifecycle mutex changes are properly scoped. All behavioral changes are intentional and tested: the missing-session error path in ApplyActivitySignal now propagates an explicit error that the HTTP layer handles as a 404, the CIPending/CIUnknown cases correctly block ready-to-merge, and the store uses a typed SQLite error code for UNIQUE-constraint detection. The only open items are a missing SSE heartbeat (operational concern, not a data correctness issue) and a dead method in one test file. No files require special attention; the SSE stream in notifications.go would benefit from a heartbeat for production proxy compatibility. Important Files Changed
Sequence DiagramsequenceDiagram
participant Agent as Agent Hook
participant LCM as Lifecycle Manager
participant NW as notify.Manager
participant DB as SQLite Store
participant Hub as notify.Hub
participant SSE as SSE Client
Agent->>LCM: ApplyActivitySignal(waiting_input)
LCM->>DB: GetSession (under m.mu)
DB-->>LCM: SessionRecord
LCM->>DB: UpdateSession (under m.mu)
LCM-->>LCM: release m.mu
LCM->>NW: Notify(NeedsInput intent)
NW->>NW: enrich(intent) → NotificationRecord
NW->>DB: CreateNotification (writeMu)
Note over DB: UNIQUE partial index dedup check
DB-->>NW: "(record, inserted=true)"
NW->>Hub: Publish(record)
Hub-->>SSE: channel send (buffered, non-blocking)
Note over LCM: SCM Poll path
LCM->>DB: ApplyPRObservation
LCM->>DB: GetSession (under m.mu, notificationIntentForCurrentSCM)
DB-->>LCM: SessionRecord
LCM-->>LCM: build SCM intent (Merged/Closed/ReadyToMerge)
LCM->>NW: Notify(SCM intent)
NW->>DB: CreateNotification (writeMu)
DB-->>NW: "(record, inserted=true/false)"
NW->>Hub: Publish(record)
Hub-->>SSE: channel send
Reviews (7): Last reviewed commit: "fix: stream notifications without cdc" | Re-trigger Greptile |
There was a problem hiding this comment.
Request changes — service boundary + persist-only "emit"
Thanks for the iteration (indexes are in good shape now: idx_notifications_status + the unique dedupe index, with project_unread/session dropped). The remaining issues are architectural, not cosmetic.
1. The notification service straddles two layers. The same notifier instance is wired into both the lifecycle reducer (startLifecycle(... notifier ...) → WithNotificationSink) and the HTTP layer (APIDeps.Notifications). So service/notification is simultaneously a controller-facing REST service (ListUnread) and a synchronous dependency of the lifecycle write path (Notify). Controller-facing services should serve HTTP REST only — Notify is a domain-write producer and doesn't belong here.
2. "Emit" is persist-only — there is no push. Notify does a single CreateNotification INSERT and returns. No SSE, no broadcaster, no fan-out. The dashboard can only see notifications by polling GET /notifications. As per our design discussion we were going to expose a SSE endpoint for the same.
3. status lost its CHECK constraint. type is DB-guarded with CHECK (type IN (...)) but status is now bare TEXT. Mirror NotificationStatus.Valid() in the schema like type does, or drop both — be consistent.
|
How have we tested this -- can you add testing details? |
Summary
Verification
Closes #179