Skip to content

feat(notifications,kilo-chat): PR 3 — notifications rewrite + push on message.created#2918

Merged
iscekic merged 19 commits intofeat/kilo-chat-migration-pr1from
feat/kilo-chat-migration-pr3
Apr 30, 2026
Merged

feat(notifications,kilo-chat): PR 3 — notifications rewrite + push on message.created#2918
iscekic merged 19 commits intofeat/kilo-chat-migration-pr1from
feat/kilo-chat-migration-pr3

Conversation

@iscekic
Copy link
Copy Markdown
Contributor

@iscekic iscekic commented Apr 29, 2026

Summary

Third PR in the kilo-chat migration stack (Phases 3 and 5 of the plan). Rewires the notifications worker as a WorkerEntrypoint and wires kilo-chat to publish a push notification whenever a message is persisted.

Stacked on: #2914 (PR 2 — DB rename channel_badge_counts → badge_counts).

Notifications service (Phase 3)

  • Adds EVENT_SERVICE service binding to the worker; drops STREAM_CHAT_API_SECRET.
  • Adds vitest scaffold (config, setup, env types) with miniflare stubs for EVENT_SERVICE and a SELF self-binding for entrypoint RPC tests.
  • Replaces NotificationChannelDO with a generic dispatchPush primitive: idempotency (1h TTL alarm-pruned), presence skip via EVENT_SERVICE.isUserInContext, token lookup, badge math, Expo dispatch, receipts queue. failed outcome leaves no idempotency key, so upstream can retry.
  • Adds WorkerEntrypoint NotificationsService.sendPushForConversation (sender-exclude + dedupe + per-recipient DO fan-out) with a pure sendPushForConversationCore for unit testability.
  • Drops the now-orphan badgeBucketForInstance helper from @kilocode/notifications (the rewritten DO no longer writes per-sandbox bucket rows).
  • Deletes services/notifications/src/routes/webhooks.ts (Stream webhook gone).
  • Layers RPC types onto EVENT_SERVICE binding and enables cloudflare:test types so pnpm typecheck is clean at HEAD.

Kilo-chat producer (Phase 5)

  • Adds kiloclaw event-context builders (kiloclawInstanceContext, kiloclawConversationContext) to @kilocode/event-service; migrates event-push.ts to use them. Single source of truth shared by producers/consumers.
  • Adds fetchSandboxLabel helper.
  • Adds NOTIFICATIONS service binding (entrypoint: NotificationsService).
  • After persisting a message.created, createMessageFor now fans out a push via NOTIFICATIONS.sendPushForConversation for non-sender human members. Failures are logged, never block the send.

⚠️ Stream webhook gap

Per the plan: deleting the Stream webhook route here means Stream messages stop generating push notifications during the soak window before PR 6's mobile cutover. Stream is being decommissioned anyway; flagging explicitly.

Test plan

  • services/notifications: pnpm typecheck — clean
  • services/notifications: pnpm test — 8/8 pass (dispatch-push + send-push-for-conversation)
  • services/kilo-chat: pnpm typecheck — clean
  • services/kilo-chat: pnpm test — 246/246 pass (incl. 3 new push-notifications cases: negative no-recipients, positive multi-human, failure isolation)
  • packages/event-service: pnpm typecheck — clean

iscekic added 14 commits April 29, 2026 17:28
The badge_counts.badge_bucket column is a free-form string. To prevent
namespace collisions as more surfaces start emitting badge updates
(per-instance today, per-conversation later), centralize bucket-key
derivation in @kilocode/notifications and route NotificationChannelDO
through it. Mirrors the presence-context builders in @kilocode/event-service.

Safe to introduce now without a data migration because PR 2's migration
already wipes badge_counts.
…-chat producer

Adds kiloclawInstanceContext and kiloclawConversationContext path
builders to @kilocode/event-service, replacing hardcoded template
literals in kilo-chat's event-push.ts and its test so all callers
share a single source of truth.
When a chat message is persisted, fire-and-forget a call to
NOTIFICATIONS.sendPushForConversation so non-sender human members of the
conversation receive a push. Runs after realtime/event-service delivery
inside postCommitFanOut, with errors swallowed so push failures cannot
fail the send.

- Skip when there are no other human recipients or no sandboxId.
- senderUserId = callerId for human senders, null for bot senders.
- title is "<sandboxLabel> · <conversationTitle>"; bodyPreview is the
  first 200 chars of the concatenated text blocks.
- Add @kilocode/notifications workspace dep and layer the RPC method
  shape into Env via bindings.d.ts.
- Add a notifications-stub worker to the vitest config so tests can
  spy on env.NOTIFICATIONS.sendPushForConversation, and globally mock
  sandbox-lookup in setup.ts (it imports pg via @kilocode/db).
@iscekic iscekic self-assigned this Apr 29, 2026
Comment thread services/notifications/src/index.ts Outdated
Comment thread services/notifications/src/index.ts
Comment thread services/notifications/src/index.ts
Comment thread services/notifications/src/dos/NotificationChannelDO.ts Outdated
Comment thread services/notifications/src/dos/NotificationChannelDO.ts Outdated
@kilo-code-bot
Copy link
Copy Markdown
Contributor

kilo-code-bot Bot commented Apr 29, 2026

Code Review Summary

Status: 2 Issues Found | Recommendation: Address before merge

Overview

Severity Count
CRITICAL 0
WARNING 2
SUGGESTION 0

Fix these issues in Kilo Cloud

Issue Details (click to expand)

WARNING

File Line Issue
services/notifications/src/index.ts 49 Conversation-scoped badge buckets still cannot be cleared by the current mobile unread/read flow.
services/notifications/src/index.ts 56 Push payload shape still does not match the deployed mobile notification parser.
Other Observations (not in diff)

Issues found in unchanged code that cannot receive inline comments:

File Line Issue
N/A N/A None
Files Reviewed (1 file)
  • packages/event-service/src/presence.ts - 0 new issues

Carried forward from previous review:

  • services/notifications/src/index.ts - 2 accepted transitional mobile compatibility warnings.

Reviewed by gpt-5.5-2026-04-23 · 692,149 tokens

iscekic added 2 commits April 29, 2026 18:53
…es, fix test mock

- Remove `stream-chat` from `services/notifications/package.json`; the Stream
  webhook (its only consumer) was deleted earlier in the stack.
- Regenerate `worker-configuration.d.ts` so the workerd runtime types match the
  current toolchain (sibling services were on `1.20260312.1`; this one had
  drifted to `1.20251217.0` from a stale local cache).
- Fix the global test mock to reference the renamed `badge_counts` table; the
  setup file was authored against the pre-rename name and never matched.
- Tidy two pre-existing lint nits in the new test files (`import type` for
  type-only import, drop unused `cols` parameter).
…leak

- Switch `NotificationsService` from default-only to a named class export
  with a separate default. `services/kilo-chat/wrangler.jsonc` binds via
  `entrypoint: "NotificationsService"`, which resolves named module
  exports. The default-only form (`export default class NotificationsService`)
  exports under the `default` key — kilo-chat's RPC binding would not have
  resolved at deploy. Mirrors the existing pattern in
  `services/kilo-chat/src/index.ts` (`KiloChatService`).

- `dispatchPush` now uses a two-stage idempotency record (`pending` →
  `delivered`). The badge increment was previously non-idempotent: an
  Expo failure returned `failed` without writing the idempotency key, so
  upstream retries (which the design explicitly invites) re-ran the
  increment before the next send and inflated the badge by one per
  retry. The `pending` marker is written before the increment and
  short-circuits the increment on retry; the `delivered` marker is only
  written on success.

- `setAlarm` is now gated on `getAlarm() === null`. Calling `setAlarm`
  unconditionally on each successful push — as the previous code did —
  replaces the pending alarm and pushes the cleanup forward indefinitely
  on a conversation receiving more than one push per `IDEM_TTL_MS`,
  leaking expired idempotency entries.

Adds two test cases covering the badge-retry and alarm-reset paths.
Comment thread services/notifications/src/dos/NotificationChannelDO.ts Outdated
Comment thread services/notifications/src/dos/NotificationChannelDO.ts
iscekic added 2 commits April 29, 2026 19:11
- Schedule the cleanup alarm when writing the `pending` marker, not only
  on `delivered`. Without this, an Expo failure followed by no further
  push activity for the conversation leaves the `pending` record in DO
  storage forever (no alarm was ever set to prune it).

- After the alarm fires, reschedule for the earliest remaining record's
  expiry instead of leaving the alarm slot empty. Otherwise a quiet
  conversation strands its younger entries until some unrelated future
  dispatch wakes the DO up.

Both paths go through a small `ensureCleanupAlarm` helper that gates on
`getAlarm() === null` so a busy conversation still doesn't push the
alarm forward on every call.
The kiloclaw-scoped presence paths are literally `/presence` prefixed
onto the kiloclaw event-context paths. Build them by composition so the
`/kiloclaw/{sandboxId}[/{conversationId}]` segment shape is defined in
exactly one place — `kiloclaw-contexts.ts`.

Pure refactor; same string output, template-literal types still narrow
to the same shape.
Base automatically changed from feat/kilo-chat-migration-pr2 to feat/kilo-chat-migration-pr1 April 30, 2026 12:53
…to feat/kilo-chat-migration-pr3

# Conflicts:
#	packages/notifications/src/badge-buckets.ts
#	pnpm-lock.yaml
#	services/notifications/package.json
#	services/notifications/src/dos/NotificationChannelDO.ts
@iscekic iscekic merged commit 8147303 into feat/kilo-chat-migration-pr1 Apr 30, 2026
1 check passed
@iscekic iscekic deleted the feat/kilo-chat-migration-pr3 branch April 30, 2026 13:01
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.

1 participant