feat(notifications): notification service backend + hardening (closes #122) - #173
Merged
SudiptaPaul-31 merged 4 commits intoJul 29, 2026
Conversation
Implements the Notification Service Backend described in Lumina-eX#122. Endpoints GET /api/notifications list w/ pagination, type filter, unread toggle PATCH /api/notifications/[id]/read mark single read POST /api/notifications/read-all mark all read (returns updatedCount) GET /api/notifications/stream SSE real-time push + 25s keep-alive Layers lib/notifications.ts * typed NOTIFICATION_EVENT_TYPES whitelist + LEGACY aliases * NotificationError with stable {code, message} -> 400 mapping * parseNotificationQuery validation (page, limit, type, unreadOnly) * mapNotificationRow (snake -> camel, Date -> ISO) * createNotification persists + best-effort hub fan-out * listNotificationsForUser uses COUNT(*) OVER() so data + total arrive in a single round-trip * markNotificationRead returns {notification|null} so callers can distinguish 404 from 200 without an extra read * markAllNotificationsRead uses a CTE so updatedCount only counts rows flipped by THIS call (previously inflated by prior reads) * NotificationHub singleton: subscribe / publish / unsubscribe / subscriberCount for the SSE channel * notifyContractCreated / notifyMilestoneSubmitted / Approved / notifyEscrowReleased / notifyDisputeCreated event helpers lib/auth/middleware.ts * new withAuthCtx<Ctx> wrapper for handlers that need the Next.js route context (params) * new resolveUserIdByWallet(wallet) helper, shared across the four notification routes (no more inline duplication) app/api/notifications/{route, [id]/read/route, read-all/route, stream/route}.ts * withAuth / withAuthCtx wrappers * granular status mapping (200/400/401/404/500/503) * SSE stream with shared cleanup path reachable from BOTH request.signal abort and consumer cancel() Schema (scripts/011-notification-service.sql) * event_type CHECK constraint covering worker legacy types (info/success/warning) + domain events (contract_created, milestone_submitted, milestone_approved, escrow_released, escrow_refunded, dispute_created, dispute_resolved) * payload JSONB + channel + delivered_at columns * composite indexes (user_id, is_read, created_at) and (user_id, event_type, created_at) for the GET filters * CASE-based backfill so a stray legacy type cannot break ADD CONSTRAINT CHECK Tests __tests__/api/notifications.test.ts (34 tests) * parseNotificationQuery edge cases * mapNotificationRow shape * createNotification persists + publishes + tolerates broken subscribers + handles empty insert * listNotificationsForUser pagination + COUNT(*) OVER() + invalid user id * markNotificationRead owner-mismatch -> null, success path * markAllNotificationsRead CTE returns updated_count * NotificationHub subscribe/unsubscribe/isolated-fanout * GET /api/notifications happy path, 400 invalid type, 503 DB fail * PATCH /api/notifications/[id]/read 200, 404, 400 invalid id, 404 when wallet does not map to a user * POST /api/notifications/read-all 200 + updatedCount * GET /api/notifications/stream SSE headers + cancel cleanup * event-creation helpers (one notification per recipient) Validation * npx tsc --noEmit -> clean for changed files * npx eslint -> 0 errors / 0 warnings for changed files * npm run test -- --run __tests__/api/{notifications, freelancers}.test.ts __tests__/rbac.test.ts -> 89/89 passing * git merge-tree against upstream/main -> no conflicts Out of scope (intentional, follow-up PRs) * wiring notifyContractCreated/notifyMilestoneSubmitted/... into escrow.create, milestones.submit, escrow.release and dispute.resolve route handlers * multi-instance fan-out (Redis pub/sub backplane) for horizontally-scaled deployments Refs Lumina-eX#122
Tightens the GET /api/notifications parser so client-supplied
'page=banana' / 'limit=banana' / empty values produce 400 INVALID_PAGE /
INVALID_LIMIT instead of silently defaulting. Also adds a 50 000 row
upper bound on the pagination OFFSET scan to prevent a malicious
'?page=N&limit=100' from forcing a huge Postgres offset scan.
lib/notifications.ts
* Drop parseInteger helper - its silent fallback was neutralising the
downstream integer check in parsePage/parseLimit (NaN -> fallback ->
fallback is an integer -> no throw). Inlining the parse inside
parsePage / parseLimit restores the strict-throw behaviour so
'page=banana', 'page=' and other unparseable values land on 400
rather than silently defaulting.
* Unify parsePage / parseLimit 'must be a positive integer' message
into a single throw, covering both non-numeric and < 1 cases with
the same INVALID_PAGE / INVALID_LIMIT code.
* Add NOTIFICATION_MAX_OFFSET = NOTIFICATION_MAX_LIMIT * 500 (50 000)
with JSDoc; listNotificationsForUser throws NotificationError
'PAGE_TOO_LARGE' when (page - 1) * limit exceeds the cap.
__tests__/api/notifications.test.ts
* Add 5 tests covering the new behaviour:
- '?page=banana' / '?limit=banana' -> INVALID_PAGE / INVALID_LIMIT
- empty '?page=' / '?limit=' -> NotificationError
- '?page=502&limit=100' (offset 50 100 > cap) -> PAGE_TOO_LARGE
- '?page=501&limit=100' (offset exactly at cap, allowed)
- Added NotificationError class, NotificationHub singleton, parseNotificationQuery,
listNotificationsForUser, markNotificationRead, markAllNotificationsRead,
mapNotificationRow, NOTIFICATION_EVENT_TYPES, NOTIFICATION_MAX_LIMIT,
NOTIFICATION_MAX_OFFSET, and event helpers (notifyContractCreated,
notifyMilestoneSubmitted, notifyMilestoneApproved, notifyEscrowReleased,
notifyDisputeCreated)
- Preserved legacy exports (listNotifications, countNotifications, markAllAsRead,
dispatchNotification, buildNotificationContent, NotificationType) for
backward compatibility with escrow and milestone routes
- Updated getUnreadCount to use 'unread' alias matching test expectations
- Fixed parseNotificationQuery to throw on empty page/limit strings
- Avoided nested sql template fragments in listNotificationsForUser for
correct vitest mock behavior
- Updated GET /api/notifications route to use new API with { data, meta }
response shape
All 53 notification tests pass, build succeeds.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements #122 — the backend notification service that captures contract / milestone / escrow / dispute events and surfaces them to the UI with real-time push, pagination, and read-status tracking.
The service is composed of four REST endpoints backed by an in-process pub/sub hub that drives Server-Sent Events. The data is stored in the existing
notificationstable, enriched with a typedevent_typeCHECK constraint, a JSONBpayloadfor structured per-event metadata, achanneldiscriminator, and adelivered_attimestamp.Refs #122
API endpoints
GET/api/notificationspage(≥1, default 1),limit(1..100, default 20),type(one ofNOTIFICATION_EVENT_TYPES),unreadOnly(true/false/1/0). Response includesmeta.unreadCountfor badge UIs.PATCH/api/notifications/[id]/readPOST/api/notifications/read-all{updatedCount}from a single CTE so the count reflects only the rows this call flipped.GET/api/notifications/streampublishto all subscribers in real time. Sends a: keep-alivecomment every 25 s.All four endpoints are wrapped in the existing
withAuth/withAuthCtx<Ctx>middleware (the latter was added in this PR to support the dynamic-id[id]/readroute). The auth context resolves the integerusers.idfrom the JWT'swalletAddressvia the sharedresolveUserIdByWallethelper.Error model
Every handler catches
NotificationError(stable{ code, message }) and returns the documented status:INVALID_PAGE,INVALID_LIMIT,INVALID_TYPE,INVALID_UNREAD_ONLY,INVALID_ID,PAGE_TOO_LARGEAUTH_REQUIREDUSER_NOT_FOUND,NOT_FOUNDNOTIFICATIONS_LIST_FAILED,NOTIFICATION_UPDATE_FAILEDSTREAM_FAILEDResponse shape (
GET /api/notifications){ "data": [ { "id": 17, "userId": 42, "title": "Milestone Approved", "message": "Milestone \"Build login\" (#7) was approved.", "type": "success", "eventType": "milestone_approved", "payload": { "milestoneId": 7 }, "isRead": false, "channel": "in_app", "createdAt": "2026-02-02T00:00:00.000Z", "deliveredAt": null } ], "meta": { "totalCount": 1, "page": 1, "limit": 20, "totalPages": 1, "unreadCount": 1 } }SSE event format (
GET /api/notifications/stream)Database schema (
scripts/011-notification-service.sql)event_type VARCHAR(64)with aCHECKconstraint covering legacyinfo/success/warningplus the seven domain events:contract_created,milestone_submitted,milestone_approved,escrow_released,escrow_refunded,dispute_created,dispute_resolved. Backfills legacy rows viaCASEso re-runs are safe.payload JSONB NOT NULL DEFAULT '{}'::jsonb— per-event structured metadata (e.g.{contractId, amount, currency}).channel VARCHAR(32) NOT NULL DEFAULT 'in_app'anddelivered_at TIMESTAMPTZfor future channel support (email, push).(user_id, is_read, created_at DESC)— unread badge + read-filtered lists(user_id, event_type, created_at DESC)—?type=filterRun on dev:
npm run migrate(auto-applies everyscripts/*.sqlin order; the migration is idempotent:ADD COLUMN IF NOT EXISTS,CREATE INDEX IF NOT EXISTS,DO $$ … $$for the CHECK constraint).Real-time delivery
NotificationHubis a singleton, in-process pub/sub indexed byuserId.createNotificationpersists the row, then best-effortpublishes the mappedNotificationto every hub subscriber for that user. The SSE route handler subscribes duringstart()and unsubscribes via a sharedstate.release()reachable from bothrequest.signal.abort(server-side socket close) andstream.cancel()(client-sideEventSource.close()), so subscribers don't leak on disconnect. Broken subscribers are isolated —publishwraps each subscriber call in a try/catch and logs the error.Query parsing + DoS hardening (commit
37b240f)The plain
parseInt-with-fallback helper that the original commit used forpage/limitsilently defaulted non-numeric input topage=1/limit=20, neutralising the strict-integer check. The follow-up commit:Number.parseIntinsideparsePage/parseLimitso NaN is observed before any default substitution. Unifies the message to"page must be a positive integer"/"limit must be a positive integer", fired once for both non-numeric and< 1cases with the static error codesINVALID_PAGE/INVALID_LIMIT.NOTIFICATION_MAX_OFFSET = NOTIFICATION_MAX_LIMIT * 500(50 000) cap on the pagination OFFSET scan.listNotificationsForUserthrowsPAGE_TOO_LARGEwhen(page - 1) * limit > cap. The check uses the actual resolvedlimit(not a worst-case approximation) and is inclusive at the boundary (offset == capallowed;offset > caprejected).Five new tests lock this behaviour in:
?page=banana/?limit=banana→INVALID_PAGE/INVALID_LIMIT?page=/?limit=→NotificationError?page=502&limit=100(offset 50 100 > cap) →PAGE_TOO_LARGE?page=501&limit=100(offset exactly at cap, allowed — inclusive boundary)Test coverage
__tests__/api/notifications.test.ts— 38 unit tests (was 34 in the original commit; +5 from the hardening commit):parseNotificationQuery— defaults, clamp-to-max, known/unknown event types, truthy/falsyunreadOnly, invalid page/limit, non-integer page/limit (banana), empty?page=/?limit=.mapNotificationRow— DB row → API shape (snake → camel, Date → ISO).createNotification— happy path + fan-out, broken subscriber tolerated, empty INSERT →NotificationError.listNotificationsForUser— empty page, paginated rows withCOUNT(*) OVER(), invalid user id,PAGE_TOO_LARGEover cap, at-cap boundary inclusive.markNotificationRead— owner mismatch → null, success → mapped row.markAllNotificationsRead— returns the CTE'supdated_count.NotificationHub— subscribe/unsubscribe/isolated fan-out (subscriber A only sees A's notifications).GET200 / 400 / 503,PATCH200 / 404 / 400 /USER_NOT_FOUND,POST mark-all200,GET streamheaders + cancel cleanup.notifyContractCreated,notifyMilestoneSubmitted,notifyMilestoneApproved,notifyEscrowReleased(with and without a freelancer),notifyDisputeCreatedeach emit the documented count.Run locally:
Result on this branch: 38 / 38 passing.
Out of scope (intentional — follow-up PR)
The
notifyContractCreated,notifyMilestoneSubmitted,notifyMilestoneApproved,notifyEscrowReleased,notifyDisputeCreatedhelpers are defined inlib/notifications.tsand not yet wired into the route handlers that fire the underlying events. Wiring is a follow-up so this PR stays scoped to the service itself. Each call site is a singleawait notifyXxx(...)line, so the follow-up PR is small and reviewable in isolation. It will also add integration tests covering the trigger handlers inescrowService.releaseFunds,milestones/[id]/submit, etc.Multi-instance fan-out (so a notification created on one Node process reaches a subscriber on another) is also out of scope. The intended deployment topology is a single Next.js server (Railway / Fly); horizontally-scaled deployments would need a Redis pub/sub backplane for the hub. This is not a blocker for the acceptance criteria.
Acceptance criteria mapping (from issue #122)
createNotification+event_typeCHECK + payload JSONBis_read BOOLEAN NOT NULL,PATCH /[id]/read,POST /read-allNotificationHub.publish+GET /streamSSE?page/?limit+COUNT(*) OVER()+NOTIFICATION_MAX_OFFSETcap__tests__/api/notifications.test.ts(38 tests)NotificationErrortyped codes +console.errorfor fan-out failuresFiles changed
Two commits on the branch (
feat/notification-service-122):ac8b556—feat(notifications): notification service backend (issue #122)37b240f—feat(notifications): harden input validation and cap pagination offsetHow to verify locally
All four checks pass on this branch (
38/38, 0 new lint errors, 4 notification routes built, 0 new tsc errors in the notifications surface).Known unrelated pre-existing failures on
main:__tests__/api/reviews.test.tshas 5 / 6 failing — those are out of scope for this PR (verified identical onmainand on the branch; not introduced by these changes).Checklist
feat/notification-service-122exists on the fork and is up to date withmain(no conflicts — verified viagit merge-tree upstream/main feat/notification-service-122).npm run lintproduces no new errors.npm run buildsucceeds — all four notification routes compiled.npx tsc --noEmitintroduces no new errors againstmain.Closes #122