Skip to content

feat: add notifications v1#181

Open
whoisasx wants to merge 7 commits into
mainfrom
feat/179
Open

feat: add notifications v1#181
whoisasx wants to merge 7 commits into
mainfrom
feat/179

Conversation

@whoisasx

Copy link
Copy Markdown
Collaborator

Summary

  • add durable notification domain/service/storage with unread dedupe
  • emit lifecycle notification intents for needs_input and SCM PR ready/merged/closed states
  • expose GET /api/v1/notifications with generated OpenAPI/frontend types

Verification

  • npm run sqlc
  • npm run api
  • npm run lint
  • npm run frontend:typecheck

Closes #179

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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_input and SCM PR state transitions (ready_to_merge, pr_merged, pr_closed_unmerged) and wire the sink in the daemon.
  • Expose GET /api/v1/notifications and 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.

Comment thread backend/internal/lifecycle/manager.go
@greptile-apps

greptile-apps Bot commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR introduces a durable notification domain for v1: lifecycle events (waiting_input, PR merged/closed/ready-to-merge) are converted into persisted NotificationRecord rows and fanned out to SSE subscribers. A partial UNIQUE index on (session_id, type, pr_url) WHERE status = 'unread' provides server-side dedup without string-matching hacks; the store layer uses a typed SQLite error code for the UNIQUE fallback.

  • lifecycle/manager.go and reactions.go gain mutex-serialized session snapshots so SCM and activity notifications don't race; ApplyActivitySignal deliberately changes missing-session behavior from a silent nil to ErrSessionNotFound, which is caught and surfaced as HTTP 404 in the sessions controller.
  • notify.Manager (write path) and service/notification.Manager (read path) are separate; the write manager enriches intents into titled/bodied records before persisting and publishing to the in-process hub.
  • GET /api/v1/notifications exposes paged unread notifications; GET /api/v1/notifications/stream is a buffered SSE stream with per-project filtering.

Confidence Score: 5/5

Safe 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

Filename Overview
backend/internal/lifecycle/manager.go ApplyActivitySignal refactored from mutate() to inline mutex management; deliberately changes missing-session behavior from nil to ErrSessionNotFound (handled in HTTP layer). Notification emit is correctly deferred outside the mutex.
backend/internal/lifecycle/reactions.go New SCM notification intent logic with mutex-serialized session snapshot; CIPending/CIUnknown correctly block ready-to-merge. Merged/closed PR notifications fire even for terminated sessions (intentional).
backend/internal/notify/manager.go Write-side notification manager; enriches intents, persists via store, publishes to hub. Duplicate handling correctly returns clean no-op. Nil-safe guards throughout.
backend/internal/notify/hub.go In-process SSE fan-out hub with buffered channels. Non-blocking Publish with silent drop is intentional (lifecycle must not block on slow subscribers). Thread-safe with RWMutex.
backend/internal/httpd/controllers/notifications.go REST list endpoint and SSE stream endpoint for notifications. Stream lacks a periodic heartbeat, risking silent proxy-level disconnects on idle connections.
backend/internal/storage/sqlite/store/notification_store.go Serialized write path with explicit pre-insert dedupe check and UNIQUE-constraint fallback; uses typed SQLite error code rather than string matching (addressing previous review concern).
backend/internal/storage/sqlite/migrations/0011_notifications.sql Clean migration with FK cascade, CHECK constraints on type/status, and partial UNIQUE index for unread dedup. Down migration drops objects in correct order.
backend/internal/service/notification/service_test.go Good coverage of list + target enrichment; fakeStore implements unused CreateNotification (satisfies a different interface, dead code in this context).

Sequence Diagram

sequenceDiagram
    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
Loading

Reviews (7): Last reviewed commit: "fix: stream notifications without cdc" | Re-trigger Greptile

Comment thread backend/internal/lifecycle/reactions.go Outdated
Comment thread backend/internal/storage/sqlite/store/notification_store.go Outdated
Comment thread backend/internal/lifecycle/reactions.go Outdated
Comment thread backend/internal/lifecycle/reactions.go Outdated
Comment thread backend/internal/storage/sqlite/migrations/0011_notifications.sql
Comment thread backend/internal/storage/sqlite/migrations/0011_notifications.sql Outdated
Comment thread backend/internal/storage/sqlite/migrations/0011_notifications.sql Outdated
Comment thread backend/internal/storage/sqlite/migrations/0011_notifications.sql Outdated
Comment thread backend/internal/service/notification/service.go
Comment thread backend/internal/httpd/controllers/dto.go Outdated
Comment thread backend/internal/lifecycle/reactions.go

@neversettle17-101 neversettle17-101 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

Comment thread backend/internal/daemon/daemon.go
Comment thread backend/internal/service/notification/service.go Outdated
Comment thread backend/internal/storage/sqlite/migrations/0011_notifications.sql Outdated
Comment thread backend/internal/cdc/event.go
@neversettle17-101

Copy link
Copy Markdown
Collaborator

How have we tested this -- can you add testing details?

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.

Design: notifications v1

3 participants