feat: add message editing/deletion support with MessageDeliveryService #86

shuhuiluo · 2025-12-05T09:27:14Z

Add message_mappings table to track GitHub entities to Towns message IDs
Create MessageDeliveryService combining thread tracking with message lifecycle
Support edit/delete actions for comments, reviews, and review comments
Add (edited) indicator to formatted messages when content is edited
Extract toDeliveryAction helper for mapping webhook actions
Replace ThreadService with unified MessageDeliveryService

🤖 Generated with Claude Code

Summary by CodeRabbit

New Features
- Shows "(edited)" on synced GitHub content (PRs, issues, comments, reviews) and updates message headers for edits.
- Supports create/edit/delete flows for synced messages for more accurate updates.
Chores
- Reworked delivery infrastructure and mapping storage for more reliable message updates and periodic cleanup of old mappings (default 30 days).

_{✏️ Tip: You can customize this high-level summary in your review settings.}

- Add message_mappings table to track GitHub entities to Towns message IDs - Create MessageDeliveryService combining thread tracking with message lifecycle - Support edit/delete actions for comments, reviews, and review comments - Add _(edited)_ indicator to formatted messages when content is edited - Extract toDeliveryAction helper for mapping webhook actions - Replace ThreadService with unified MessageDeliveryService 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>

coderabbitai · 2025-12-05T09:27:27Z

Walkthrough

This PR replaces the thread-based delivery model with a message-mapping delivery model. It adds a new message_mappings table and Drizzle migration/snapshot, removes the ThreadService and event_threads usage, and introduces MessageDeliveryService to handle create/edit/delete flows, mapping storage, and expiry-based cleanup. EventProcessor is refactored to delegate delivery to the new service (constructor signature changed) and webhook formatters now annotate edited events. Application wiring in src/index.ts is updated to instantiate and use MessageDeliveryService. A new constant MESSAGE_MAPPING_EXPIRY_DAYS is added.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

New MessageDeliveryService implementation: complex lifecycle logic (create/edit/delete), optimistic update checks, mapping persistence, expiry cleanup.
EventProcessor refactor: constructor/signature change and mapping of GitHub actions to delivery actions across many event handlers.
Migration and schema additions: review composite PK, CHECK constraints, index, and updated Drizzle snapshot for correctness.
Wiring changes in src/index.ts and removal of ThreadService: ensure startup/cleanup and dependency injection are correct.
Webhook formatter changes: confirm edited handling is consistent across formats.

Files/areas to pay special attention to:

src/services/message-delivery-service.ts (core logic, DB interactions, error handling)
src/github-app/event-processor.ts (action mapping, parameter passing, dropped thread logic)
drizzle/0007_tiresome_glorian.sql and drizzle/meta/0007_snapshot.json (schema correctness and constraints)
src/index.ts (service instantiation and lifecycle calls)
src/formatters/webhook-events.ts (edited vs created message rendering)

Possibly related PRs

feat: add thread-based event grouping for PRs and issues #75 — Touches the same messaging/threading domain; adds ThreadService/event_threads while this PR replaces that approach with message_mappings/MessageDeliveryService.
feat: add branch-specific event filtering #70 — Modifies src/github-app/event-processor.ts and event handling/delivery flow; directly related to the EventProcessor changes here.
refactor: move database layer to Postgres with drizzle #28 — Changes DB schema and Drizzle migrations; related to the new message_mappings table and snapshot updates.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The PR title accurately summarizes the main change: adding message editing/deletion support via MessageDeliveryService, which aligns with the primary objectives of replacing ThreadService and implementing edit/delete actions.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch feat/message-delivery-service

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2e8a6d1 and ce71e25.

📒 Files selected for processing (1)

src/services/message-delivery-service.ts (1 hunks)

🧰 Additional context used

📓 Path-based instructions (2)

**/*.ts

📄 CodeRabbit inference engine (AGENTS.md)

**/*.ts: Store context externally - maintain stateless bot architecture with no message history, thread context, or conversation memory
Use <@{userId}> for mentions in messages AND add mentions in sendMessage options - do not use @username format
Implement event handlers for onMessage, onSlashCommand, onReaction, onTip, and onInteractionResponse to respond to Towns Protocol events
Define slash commands in src/commands.ts as a const array with name and description properties, then register handlers using bot.onSlashCommand()
Set ID in interaction requests and match ID in responses to correlate form submissions, button clicks, and transaction/signature responses
Use readContract for reading smart contract state, writeContract for SimpleAccount operations, and execute() for external contract interactions
Fund bot.appAddress (Smart Account) for on-chain operations, not bot.botId (Gas Wallet/EOA)
Use bot.* handler methods directly (outside event handlers) for unprompted messages via webhooks, timers, or tasks - requires channelId, spaceId, or other context stored externally
Always check permissions using handler.hasAdminPermission() before performing admin operations like ban, redact, or pin
User IDs are hex addresses in format 0x..., not usernames - use them consistently throughout event handling and message sending
Slash commands do not trigger onMessage - register slash command handlers using bot.onSlashCommand() instead
Use getSmartAccountFromUserId() to retrieve a user's wallet address from their userId
Include required environment variables: APP_PRIVATE_DATA (bot credentials) and JWT_SECRET (webhook security token)

Files:

src/services/message-delivery-service.ts

**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (AGENTS.md)

Provide alt text for image attachments and use appropriate MIME types for chunked attachments (videos, screenshots)

Files:

src/services/message-delivery-service.ts

🧠 Learnings (1)

📚 Learning: 2025-11-25T03:24:12.463Z

Learnt from: CR
Repo: HereNotThere/bot-github PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T03:24:12.463Z
Learning: Applies to **/*.ts : Implement event handlers for onMessage, onSlashCommand, onReaction, onTip, and onInteractionResponse to respond to Towns Protocol events

Applied to files:

src/services/message-delivery-service.ts

🧬 Code graph analysis (1)

src/services/message-delivery-service.ts (4)

src/types/bot.ts (1)

TownsBot (8-8)

src/db/index.ts (1)

db (57-57)

src/db/schema.ts (2)

eventThreads (288-323)

messageMappings (237-282)

src/constants.ts (1)

MESSAGE_MAPPING_EXPIRY_DAYS (101-101)

🔇 Additional comments (8)

src/services/message-delivery-service.ts (8)

1-50: LGTM! Clean type definitions and imports.

The type definitions are well-structured and properly documented. The interfaces clearly separate concerns (threading, entity tracking, delivery parameters) and align with the database schema.

65-145: Control flow correctly prevents unintended message creation.

The switch statement with explicit early returns ensures that delete and edit actions will never fall through to message creation when entityContext is missing. This addresses the concern from the previous review.

147-175: LGTM! Delete flow is clean and handles missing mappings gracefully.

The method correctly handles the case where no mapping exists (silent no-op), which is appropriate for delete actions.

177-232: LGTM! Edit flow includes optimistic concurrency control.

The shouldUpdate check (lines 200-216) correctly prevents stale edits by comparing GitHub timestamps. The mapping is refreshed with the new timestamp and expiry, maintaining accurate state.

234-269: LGTM! Create flow correctly sequences operations.

The method properly sends the message before storing metadata and conditions storage on successful message creation (eventId presence). While there's no transaction wrapping the storage operations, this is acceptable given the external API dependency.

279-339: LGTM! Thread management handles expiry and conflicts correctly.

The expiry check in getThreadId (line 296) filters expired threads at query time, and storeThread properly refreshes the expiry window on updates while preserving createdAt.

349-461: LGTM! Message mapping operations maintain consistency.

All operations use the consistent composite key, expiry checks prevent stale data at query time, and shouldUpdate provides optimistic concurrency control. The onConflictDoUpdate strategy correctly refreshes timestamps and expiry while preserving creation time.

471-537: LGTM! Cleanup implementation is robust and observable.

The parallel cleanup execution (line 474) is efficient, error handling covers both immediate and periodic runs, and returning the interval handle allows the caller to control the lifecycle. The immediate cleanup on startup ensures expired records are removed quickly without blocking initialization.

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 1

🧹 Nitpick comments (2)

src/services/message-delivery-service.ts (2)

261-321: Timestamp / expiry behavior is sound; consider minor refinements for consistency

The thread/message mapping helpers look solid: they key off the composite identifiers, gate lookups on expiresAt, and use onConflictDoUpdate to keep mappings current. That aligns well with the DB schema you defined and keeps state external to the bot process, as per the stateless-architecture guideline. Based on learnings, this is exactly the right direction.

Two small, optional refinements:

Reuse a single now where practical

In several places you do:
- gt(..., new Date()) in getThreadId / getMessageId
- const now = new Date(); in storeThread / storeMapping
- const now = new Date(); in cleanupExpired()
The tiny time skew between these calls is unlikely to matter, but if you ever refactor or unit test around boundaries, passing a now: Date down (or computing it once per call) keeps semantics perfectly consistent.
Be explicit about “expired but not yet cleaned up” behavior

Because lookups use gt(expiresAt, now) and cleanup uses lt(expiresAt, now), there’s an intentional “expired but still present” window where rows:
- Are ignored by getThreadId / getMessageId
- But have not yet been deleted by cleanupExpired()
That’s a reasonable design (no edits/deletes to very old messages; periodic janitor cleans them up), but it may be worth a short comment near expiresAt calculation or getMessageId/getThreadId explaining that this is intentional, so future maintainers don’t try to “fix” the inequality asymmetry.

These are polish-level suggestions only; the current logic is functionally correct.

Also applies to: 352-420

448-513: Cleanup/interval flow looks good; consider minor ergonomics improvements

The cleanup utilities are straightforward and correct: you delete expired threads and message mappings and log aggregate counts, then schedule a periodic job via startPeriodicCleanup(). That fits the new persistence model nicely.

A couple of optional tweaks you might consider:

Run deletes in parallel

cleanupExpired() currently does two await db.delete(...) calls sequentially. They’re independent, so you could shave a bit of latency by running them together:

const threadResults = await db
```
 .delete(eventThreads)
```

 .where(lt(eventThreads.expiresAt, now))

```
 .returning({ id: eventThreads.id });
```
const messageResults = await db
```
 .delete(messageMappings)
```

 .where(lt(messageMappings.expiresAt, now))

```
 .returning({
```

   channelId: messageMappings.channelId,

   entityId: messageMappings.githubEntityId,

```
 });
```

const [threadResults, messageResults] = await Promise.all([
```
 db
```
```
   .delete(eventThreads)
```

   .where(lt(eventThreads.expiresAt, now))

   .returning({ id: eventThreads.id }),

```
 db
```
```
   .delete(messageMappings)
```

   .where(lt(messageMappings.expiresAt, now))

```
   .returning({
```

     channelId: messageMappings.channelId,

     entityId: messageMappings.githubEntityId,

```
   }),
```
]);


Not critical, but it keeps the job fast even as tables grow.

2. **Document lifecycle of the interval handle**

You already return `ReturnType<typeof setInterval>` so callers can clear it. A brief comment in this method’s JSDoc noting that the caller is responsible for calling `clearInterval()` on shutdown would make that contract explicit and avoid accidental timer leaks.

These are nice-to-haves; the current implementation is already serviceable.

</blockquote></details>

</blockquote></details>

<details>
<summary>📜 Review details</summary>

**Configuration used**: CodeRabbit UI

**Review profile**: CHILL

**Plan**: Pro

<details>
<summary>📥 Commits</summary>

Reviewing files that changed from the base of the PR and between 1e69cd6fe6f744911466c13ee910e079ce1eddf2 and 2e8a6d11f4b299d11815c0240dc72b80b9fb7ab5.

</details>

<details>
<summary>📒 Files selected for processing (10)</summary>

* `drizzle/0007_tiresome_glorian.sql` (1 hunks)
* `drizzle/meta/0007_snapshot.json` (1 hunks)
* `drizzle/meta/_journal.json` (1 hunks)
* `src/constants.ts` (1 hunks)
* `src/db/schema.ts` (1 hunks)
* `src/formatters/webhook-events.ts` (3 hunks)
* `src/github-app/event-processor.ts` (15 hunks)
* `src/index.ts` (3 hunks)
* `src/services/message-delivery-service.ts` (1 hunks)
* `src/services/thread-service.ts` (0 hunks)

</details>

<details>
<summary>💤 Files with no reviewable changes (1)</summary>

* src/services/thread-service.ts

</details>

<details>
<summary>🧰 Additional context used</summary>

<details>
<summary>📓 Path-based instructions (2)</summary>

<details>
<summary>**/*.ts</summary>


**📄 CodeRabbit inference engine (AGENTS.md)**

> `**/*.ts`: Store context externally - maintain stateless bot architecture with no message history, thread context, or conversation memory
> Use `<@{userId}>` for mentions in messages AND add mentions in sendMessage options - do not use @username format
> Implement event handlers for onMessage, onSlashCommand, onReaction, onTip, and onInteractionResponse to respond to Towns Protocol events
> Define slash commands in src/commands.ts as a const array with name and description properties, then register handlers using bot.onSlashCommand()
> Set ID in interaction requests and match ID in responses to correlate form submissions, button clicks, and transaction/signature responses
> Use readContract for reading smart contract state, writeContract for SimpleAccount operations, and execute() for external contract interactions
> Fund bot.appAddress (Smart Account) for on-chain operations, not bot.botId (Gas Wallet/EOA)
> Use bot.* handler methods directly (outside event handlers) for unprompted messages via webhooks, timers, or tasks - requires channelId, spaceId, or other context stored externally
> Always check permissions using handler.hasAdminPermission() before performing admin operations like ban, redact, or pin
> User IDs are hex addresses in format 0x..., not usernames - use them consistently throughout event handling and message sending
> Slash commands do not trigger onMessage - register slash command handlers using bot.onSlashCommand() instead
> Use getSmartAccountFromUserId() to retrieve a user's wallet address from their userId
> Include required environment variables: APP_PRIVATE_DATA (bot credentials) and JWT_SECRET (webhook security token)

Files:
- `src/constants.ts`
- `src/formatters/webhook-events.ts`
- `src/index.ts`
- `src/services/message-delivery-service.ts`
- `src/db/schema.ts`
- `src/github-app/event-processor.ts`

</details>
<details>
<summary>**/*.{ts,tsx,js,jsx}</summary>


**📄 CodeRabbit inference engine (AGENTS.md)**

> Provide alt text for image attachments and use appropriate MIME types for chunked attachments (videos, screenshots)

Files:
- `src/constants.ts`
- `src/formatters/webhook-events.ts`
- `src/index.ts`
- `src/services/message-delivery-service.ts`
- `src/db/schema.ts`
- `src/github-app/event-processor.ts`

</details>

</details><details>
<summary>🧠 Learnings (3)</summary>

<details>
<summary>📚 Learning: 2025-11-25T03:24:12.463Z</summary>

Learnt from: CR
Repo: HereNotThere/bot-github PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T03:24:12.463Z
Learning: Applies to **/.ts : Use bot. handler methods directly (outside event handlers) for unprompted messages via webhooks, timers, or tasks - requires channelId, spaceId, or other context stored externally


**Applied to files:**
- `src/index.ts`
- `src/github-app/event-processor.ts`

</details>
<details>
<summary>📚 Learning: 2025-11-25T03:24:12.463Z</summary>

Learnt from: CR
Repo: HereNotThere/bot-github PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T03:24:12.463Z
Learning: Applies to **/*.ts : Implement event handlers for onMessage, onSlashCommand, onReaction, onTip, and onInteractionResponse to respond to Towns Protocol events


**Applied to files:**
- `src/services/message-delivery-service.ts`

</details>
<details>
<summary>📚 Learning: 2025-11-18T23:35:49.436Z</summary>

Learnt from: shuhuiluo
Repo: HereNotThere/bot-github PR: 26
File: src/db/index.ts:61-69
Timestamp: 2025-11-18T23:35:49.436Z
Learning: In webhook_deliveries table (src/db/index.ts), the installation_id column should NOT have a FOREIGN KEY constraint because the table serves as an immutable audit log for idempotency tracking. Records must persist independently even after installations are deleted, and a foreign key would create race conditions when webhooks arrive before installation records are created. The field is intentionally nullable to support webhooks without installation context.


**Applied to files:**
- `drizzle/meta/0007_snapshot.json`

</details>

</details><details>
<summary>🧬 Code graph analysis (2)</summary>

<details>
<summary>src/services/message-delivery-service.ts (4)</summary><blockquote>

<details>
<summary>src/types/bot.ts (1)</summary>

* `TownsBot` (8-8)

</details>
<details>
<summary>src/db/index.ts (1)</summary>

* `db` (57-57)

</details>
<details>
<summary>src/db/schema.ts (2)</summary>

* `eventThreads` (288-323)
* `messageMappings` (237-282)

</details>
<details>
<summary>src/constants.ts (1)</summary>

* `MESSAGE_MAPPING_EXPIRY_DAYS` (101-101)

</details>

</blockquote></details>
<details>
<summary>src/github-app/event-processor.ts (1)</summary><blockquote>

<details>
<summary>src/services/message-delivery-service.ts (4)</summary>

* `MessageDeliveryService` (58-514)
* `ThreadingContext` (21-25)
* `EntityContext` (31-37)
* `DeliveryAction` (9-9)

</details>

</blockquote></details>

</details>

</details>

<details>
<summary>🔇 Additional comments (15)</summary><blockquote>

<details>
<summary>drizzle/meta/_journal.json (1)</summary><blockquote>

`53-60`: **LGTM!**

The new journal entry follows the established pattern with sequential index and consistent version formatting.

</blockquote></details>
<details>
<summary>src/constants.ts (1)</summary><blockquote>

`96-101`: **LGTM!**

The constant is well-documented and provides a clear, configurable expiry period for message mappings.

</blockquote></details>
<details>
<summary>src/index.ts (2)</summary><blockquote>

`52-59`: **LGTM!**

The wiring correctly replaces ThreadService with MessageDeliveryService. The EventProcessor now receives the delivery service for centralized message lifecycle management.

---

`242-244`: **LGTM!**

Periodic cleanup is correctly wired for the new delivery service with appropriate logging.

</blockquote></details>
<details>
<summary>src/formatters/webhook-events.ts (3)</summary><blockquote>

`170-191`: **LGTM!**

The edit handling is correctly implemented with the `_(edited)_` indicator for both thread replies and standalone messages, with appropriate header differentiation.

---

`202-230`: **LGTM!**

PR review formatting correctly handles both "submitted" and "edited" actions with consistent indicator placement.

---

`241-262`: **LGTM!**

Review comment formatting follows the same pattern as issue comments, maintaining consistency across the codebase.

</blockquote></details>
<details>
<summary>drizzle/meta/0007_snapshot.json (1)</summary><blockquote>

`578-692`: **LGTM!**

The schema snapshot correctly captures the `message_mappings` table with the composite primary key, check constraints, and expiry index. The absence of foreign keys is appropriate as mappings should persist independently for audit purposes.

</blockquote></details>
<details>
<summary>drizzle/0007_tiresome_glorian.sql (1)</summary><blockquote>

`1-18`: **LGTM!**

The migration correctly creates the `message_mappings` table with appropriate constraints and an index for efficient expiry-based cleanup.

</blockquote></details>
<details>
<summary>src/db/schema.ts (1)</summary><blockquote>

`233-282`: **LGTM!**

The `messageMappings` table schema is well-structured with:
- Appropriate composite primary key for unique entity identification
- Validation constraints for entity and parent types
- Expiry index for efficient cleanup queries
- Nullable `githubUpdatedAt` for ordering webhooks and preventing stale overwrites

</blockquote></details>
<details>
<summary>src/github-app/event-processor.ts (5)</summary><blockquote>

`49-53`: **LGTM!**

The constructor is simplified to only depend on `SubscriptionService` and `MessageDeliveryService`, removing the direct bot dependency. The delivery service now handles all message sending internally.

---

`114-127`: **LGTM!**

The delivery delegation is clean - formatters are wrapped to accept `isThreadReply` and the service handles all threading, editing, and deletion logic. Using `Promise.allSettled` ensures one channel failure doesn't block others.

---

`229-257`: **LGTM!**

Issue comment handling correctly:
- Uses `toDeliveryAction` for action mapping
- Early returns for unsupported actions
- Provides complete `entityContext` with parent PR/issue information
- Correctly determines parent type based on `pull_request` field presence

---

`264-293`: **Consider the edge case for `review.submitted_at`.**

The `submitted_at` field might be `null` for draft reviews or certain edge cases. Using `Date.now()` as a fallback is reasonable, but this could cause ordering issues if webhooks arrive out of order. Verify this behavior is acceptable for your use case.

---

`397-404`: **LGTM!**

The `toDeliveryAction` helper provides a clean mapping from GitHub webhook actions to delivery actions. Returning `null` for unsupported actions enables early exit in handlers.

</blockquote></details>

</blockquote></details>

</details>

<!-- This is an auto-generated comment by CodeRabbit for review status -->

src/services/message-delivery-service.ts

- Use switch statement for explicit action branching, preventing delete/edit without entityContext from falling through to create - Add comments explaining gt/lt expiry asymmetry in lookups vs cleanup - Parallelize cleanup deletes with Promise.all 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>

coderabbitai bot reviewed Dec 5, 2025

View reviewed changes

src/services/message-delivery-service.ts Show resolved Hide resolved

shuhuiluo merged commit f141837 into main Dec 5, 2025
2 checks passed

shuhuiluo deleted the feat/message-delivery-service branch December 5, 2025 10:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat: add message editing/deletion support with MessageDeliveryService #86

feat: add message editing/deletion support with MessageDeliveryService #86

Uh oh!

shuhuiluo commented Dec 5, 2025 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Dec 5, 2025 •

edited

Loading

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

feat: add message editing/deletion support with MessageDeliveryService #86

feat: add message editing/deletion support with MessageDeliveryService #86

Uh oh!

Conversation

shuhuiluo commented Dec 5, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Dec 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Estimated code review effort

Possibly related PRs

Pre-merge checks and finishing touches

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

shuhuiluo commented Dec 5, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Dec 5, 2025 •

edited

Loading