Skip to content

feat: add notifications v1#181

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

feat: add notifications v1#181
whoisasx wants to merge 3 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 adds a durable notification domain to the backend — new notifications SQLite table with a partial unique index for unread-dedupe, a notification service layer (enrich/store/dispatch), lifecycle wiring for needs_input and SCM PR state transitions, and a GET /api/v1/notifications endpoint with generated OpenAPI and frontend TypeScript types.

  • Notification domain: domain.NotificationRecord, service Manager.Notify/ListUnread, SQLite-backed store with check-then-insert dedupe under writeMu, and a no-op DashboardDispatcher placeholder for a future SSE publisher.
  • Lifecycle wiring: ApplyActivitySignal now emits a NeedsInput intent inside the write lock; ApplySCMObservation computes SCM intents behind a separate m.mu-guarded GetSession call to prevent races with concurrent activity transitions.
  • HTTP surface: NotificationsController with query parsing, a 501 fallback for a nil service, and a typed NotificationTarget navigation hint in responses.

Confidence Score: 5/5

Safe to merge. The notification domain, storage, service, and HTTP endpoint are well-structured with good test coverage across all layers.

The core write path (dedupe under writeMu + isSQLiteUnique fallback using a stable error code) is solid. Lifecycle lock ordering is correct — activity signals hold m.mu across the full GetSession+UpdateSession+intent-build window, and SCM notifications re-acquire m.mu in a separate call after the PR observation write. All new code paths are exercised by unit tests including edge cases like duplicate suppression, terminated-session guards, CIPending/CIUnknown blocking, and the nil-service 501 fallback.

No files require special attention.

Important Files Changed

Filename Overview
backend/internal/lifecycle/manager.go ApplyActivitySignal inlined from mutate() to hold m.mu across GetSession+UpdateSession+intent-build; NeedsInput intent emitted after unlock. Activity signal now returns ErrSessionNotFound for missing sessions.
backend/internal/lifecycle/reactions.go SCM notifications now computed under m.mu via notificationIntentForCurrentSCM. scmObservationIsReadyToMerge correctly rejects CIPending/CIUnknown. Minor: Closed && !Merged redundancy (noted in previous thread).
backend/internal/storage/sqlite/store/notification_store.go Uses check-then-insert with writeMu plus isSQLiteUnique fallback (SQLITE_CONSTRAINT_UNIQUE error code, not string matching) for dedupe. Error code approach is stable.
backend/internal/service/notification/service.go Core notification service: Notify() enriches intents, delegates dedupe to the store, and dispatches; ListUnread() paginates with project filtering. Logic is clean and well-tested.
backend/internal/httpd/controllers/notifications.go Clean controller with nil-service 501 guard, query parsing, and limit capping. Status=read returns 400 as documented.
backend/internal/domain/notification.go Domain types for notification kinds and status with Validate() checks; well-structured.
backend/internal/storage/sqlite/migrations/0011_notifications.sql Creates notifications table with FK cascades, check constraints, appropriate indexes, and a partial unique index for unread dedupe.
backend/internal/service/notification/enrich.go Builds title/body from intent fields; validates PRURL requirement per type. No issues.
frontend/src/api/schema.ts Generated TypeScript types for notifications endpoint; prUrl is required (matching the schema) and NotificationTarget is correctly typed.

Sequence Diagram

sequenceDiagram
    participant Agent
    participant SessionsController
    participant LifecycleManager
    participant NotificationService
    participant SQLiteStore
    participant DashboardDispatcher

    Agent->>SessionsController: "POST /sessions/{id}/activity (state=waiting_input)"
    SessionsController->>LifecycleManager: ApplyActivitySignal()
    LifecycleManager->>SQLiteStore: GetSession() [under m.mu]
    LifecycleManager->>SQLiteStore: UpdateSession() [under m.mu]
    Note over LifecycleManager: Build NeedsInput intent if transition
    LifecycleManager->>LifecycleManager: m.mu.Unlock()
    LifecycleManager->>NotificationService: Notify(NeedsInput intent)
    NotificationService->>NotificationService: enrich() → title/body
    NotificationService->>SQLiteStore: getUnreadNotificationByDedupe() [under writeMu]
    SQLiteStore-->>NotificationService: (none)
    NotificationService->>SQLiteStore: CreateNotification()
    SQLiteStore-->>NotificationService: (created row)
    NotificationService->>DashboardDispatcher: Dispatch(notification)
    DashboardDispatcher-->>NotificationService: no-op (nil publisher)

    Note over LifecycleManager,NotificationService: SCM path
    LifecycleManager->>LifecycleManager: ApplyPRObservation()
    LifecycleManager->>LifecycleManager: notificationIntentForCurrentSCM() [under m.mu]
    LifecycleManager->>NotificationService: Notify(ReadyToMerge/PRMerged/PRClosed intent)

    Note over SessionsController,SQLiteStore: Read path
    SessionsController->>NotificationsController: "GET /api/v1/notifications?projectId=X"
    NotificationsController->>NotificationService: ListUnread(filter)
    NotificationService->>SQLiteStore: ListUnreadNotificationsByProject()
    SQLiteStore-->>NotificationService: []NotificationRecord
    NotificationService-->>NotificationsController: []Notification (with Target)
    NotificationsController-->>SessionsController: 200 ListNotificationsResponse
Loading

Reviews (3): Last reviewed commit: "fix: require passing CI for merge-ready ..." | 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
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

2 participants