Skip to content

Conversation

@shuhuiluo
Copy link
Collaborator

@shuhuiluo shuhuiluo commented Jan 14, 2026

Since channelId is globally unique in Towns, spaceId was redundant for subscription queries. This change:

  • Makes spaceId nullable in database schema (oauth_states, github_subscriptions, pending_subscriptions)
  • Updates unique indexes to use only channelId + repoFullName
  • Removes spaceId from function signatures and WHERE clauses
  • Simplifies service calls throughout the codebase

DMs now work because spaceId can be null without breaking queries. Space subscriptions continue to store spaceId for reference.

Summary by CodeRabbit

Release Notes

  • New Features

    • Added support for paid commands with USDC pricing
    • Enhanced bot initialization to return a customizable Hono app
    • Improved type safety for Direct Messages vs Space channel events with discriminated union types
  • Improvements

    • Simplified database setup using Bun CLI commands (bun db:up / bun db:down)
    • Better handling of Direct Message scenarios in event handling
  • Documentation

    • Updated configuration guidance with type narrowing examples

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

@coderabbitai
Copy link

coderabbitai bot commented Jan 14, 2026

Walkthrough

This PR introduces support for Direct Messages (DMs) by making spaceId optional throughout the codebase. Changes include: making spaceId nullable in database tables (oauthStates, githubSubscriptions, pendingSubscriptions, messageMappings), removing spaceId from uniqueness constraints and primary key definitions, updating service method signatures to remove or make spaceId optional, and adding type discriminators in documentation for DM versus Space channel payloads. A new database migration reflects schema modifications, and all call sites are updated to align with the revised signatures.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~30 minutes

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'feat: add DM support by removing spaceId from query logic' accurately summarizes the main change: enabling DM support through schema and query logic refactoring.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)
src/services/subscription-service.ts (1)

595-628: Return type mismatch: spaceId can be null but is typed as string.

The getRepoSubscribers return type declares spaceId: string (line 600), but space_id is nullable in the database schema (defined as nullable for DM channels). This creates a type mismatch:

  1. Return type issue: Change spaceId: string to spaceId: string | null in the Promise return type.

  2. Caller issue in event-processor.ts: The method passes spaceId directly to messageDeliveryService.deliver() (line 135), which expects spaceId: string (non-nullable in DeliveryParams interface). When spaceId is null, this passes a null value to functions expecting a non-null string (including getMessageId() which also expects non-nullable string). The caller must filter out subscriptions with null spaceId or update DeliveryParams to accept nullable values.

Note: polling-service.ts only uses channelId from the results, so it is unaffected.

src/handlers/github-subscription-handler.ts (2)

92-92: Fix ESLint error: unused spaceId variable.

The pipeline reports spaceId is destructured but never used in handleSubscribe. Looking at the code flow, spaceId is used by handleNewSubscription and handleUpdateSubscription which receive the full event object and destructure spaceId themselves.

Proposed fix
 async function handleSubscribe(
   handler: BotHandler,
   event: SlashCommandEvent,
   subscriptionService: SubscriptionService,
   oauthService: GitHubOAuthService,
   repoArg: string | undefined
 ): Promise<void> {
-  const { channelId, spaceId, args } = event;
+  const { channelId, args } = event;

308-308: Fix ESLint error: unused spaceId variable.

The pipeline reports spaceId is destructured but never used in handleUnsubscribe. The called functions (handleRemoveEventTypes, handleFullUnsubscribe) receive the full event object and destructure spaceId themselves.

Proposed fix
 async function handleUnsubscribe(
   handler: BotHandler,
   event: SlashCommandEvent,
   subscriptionService: SubscriptionService,
   oauthService: GitHubOAuthService,
   repoArg: string | undefined
 ): Promise<void> {
-  const { channelId, spaceId, args } = event;
+  const { channelId, args } = event;
🤖 Fix all issues with AI agents
In `@CONTRIBUTING.md`:
- Around line 63-70: The Markdown table under the environment variables is
malformed: the header separator row has an extra column separator and the shell
pipeline in the GITHUB_APP_PRIVATE_KEY_BASE64 “How to Get” cell uses an
unescaped pipe which Markdown treats as a column delimiter. Fix by making the
header separator match the three columns used by the rows (ensure exactly three
column separators in the header separator row) and escape the pipe in the
command for GITHUB_APP_PRIVATE_KEY_BASE64 (replace the raw | in `base64 -i
key.pem | tr -d '\n'` with an escaped pipe) so the table renders correctly.
🧹 Nitpick comments (1)
AGENTS.md (1)

113-119: Consider formatting the URL as a proper Markdown link.

The bare URL on line 115 should be formatted as a Markdown link for consistency and to satisfy linting rules.

📝 Suggested fix
 #### Paid Commands
 
-Add a `paid` property to your command definition with a price in USDC:
+Add a `paid` property to your command definition with a [price in USDC](https://hono.dev/docs/guides/best-practices):

Or if the URL is not meant to be there (it seems misplaced - it's about Hono best practices, not paid commands):

 #### Paid Commands
 
-Add a `paid` property to your command definition with a price in USDC:
+Add a `paid` property to your command definition with a price in USDC:

Actually, looking more closely, the URL reference to Hono best practices on line 361 makes sense there, but on line 115 it appears to be a documentation artifact that doesn't belong in the Paid Commands section. Please verify if this URL reference should be here or if it was accidentally included.

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 461381c and d8944ca.

⛔ Files ignored due to path filters (1)
  • bun.lock is excluded by !**/*.lock
📒 Files selected for processing (14)
  • AGENTS.md
  • CONTRIBUTING.md
  • README.md
  • drizzle/0009_cool_mad_thinker.sql
  • drizzle/meta/0009_snapshot.json
  • drizzle/meta/_journal.json
  • package.json
  • src/db/schema.ts
  • src/handlers/github-subscription-handler.ts
  • src/routes/oauth-callback.ts
  • src/services/github-oauth-service.ts
  • src/services/subscription-service.ts
  • src/utils/oauth-helpers.ts
  • tests/unit/handlers/github-subscription-handler.test.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.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/db/schema.ts
  • src/services/github-oauth-service.ts
  • src/handlers/github-subscription-handler.ts
  • src/utils/oauth-helpers.ts
  • src/routes/oauth-callback.ts
  • tests/unit/handlers/github-subscription-handler.test.ts
  • src/services/subscription-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/db/schema.ts
  • src/services/github-oauth-service.ts
  • src/handlers/github-subscription-handler.ts
  • src/utils/oauth-helpers.ts
  • src/routes/oauth-callback.ts
  • tests/unit/handlers/github-subscription-handler.test.ts
  • src/services/subscription-service.ts
AGENTS.md

📄 CodeRabbit inference engine (CLAUDE.md)

Document agent capabilities, responsibilities, and interfaces in AGENTS.md

Files:

  • AGENTS.md
🧠 Learnings (9)
📚 Learning: 2025-11-18T23:35:49.436Z
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:

  • src/db/schema.ts
  • drizzle/meta/0009_snapshot.json
📚 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: All event handlers receive a base payload including userId, spaceId, channelId, eventId, and createdAt - use eventId as threadId/replyId when responding to maintain event threading

Applied to files:

  • src/db/schema.ts
  • AGENTS.md
📚 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 : 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/handlers/github-subscription-handler.ts
  • src/utils/oauth-helpers.ts
  • AGENTS.md
📚 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 : Include required environment variables: APP_PRIVATE_DATA (bot credentials) and JWT_SECRET (webhook security token)

Applied to files:

  • CONTRIBUTING.md
  • package.json
📚 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:

  • AGENTS.md
📚 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 : User IDs are hex addresses in format 0x..., not usernames - use them consistently throughout event handling and message sending

Applied to files:

  • AGENTS.md
📚 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 : Slash commands do not trigger onMessage - register slash command handlers using bot.onSlashCommand() instead

Applied to files:

  • AGENTS.md
📚 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 : Define slash commands in src/commands.ts as a const array with name and description properties, then register handlers using bot.onSlashCommand()

Applied to files:

  • AGENTS.md
📚 Learning: 2025-11-17T23:50:17.552Z
Learnt from: shuhuiluo
Repo: HereNotThere/bot-github PR: 26
File: src/github-app/app.ts:1-3
Timestamp: 2025-11-17T23:50:17.552Z
Learning: The HereNotThere/bot-github project uses Bun as its package manager, with dependencies locked in bun.lock rather than package-lock.json.

Applied to files:

  • package.json
🧬 Code graph analysis (1)
src/services/subscription-service.ts (1)
src/db/schema.ts (1)
  • githubSubscriptions (71-110)
🪛 GitHub Actions: CI
src/handlers/github-subscription-handler.ts

[error] 92-92: ESLint: 'spaceId' is assigned a value but never used. (no-unused-vars) | Command: eslint --format unix ./src --max-warnings=0


[error] 308-308: ESLint: 'spaceId' is assigned a value but never used. (no-unused-vars) | Command: eslint --format unix ./src --max-warnings=0

🪛 markdownlint-cli2 (0.18.1)
AGENTS.md

115-115: Bare URL used

(MD034, no-bare-urls)

🔇 Additional comments (44)
drizzle/meta/_journal.json (1)

67-74: LGTM!

The new migration journal entry follows the established conventions: sequential idx, consistent version, and proper naming pattern for the tag. This correctly tracks the schema migration for making spaceId nullable and updating the unique indexes.

README.md (1)

164-176: LGTM!

The updated documentation clearly explains the new bun-based workflow for local PostgreSQL setup. The note about data persistence via named volume is a helpful addition for developers.

CONTRIBUTING.md (1)

27-29: LGTM!

The setup commands are clear and align with the new bun-based workflow defined in package.json.

package.json (3)

3-3: LGTM!

Version bump to 1.1.0 is appropriate for this feature addition (DM support).


27-35: LGTM!

Dependency updates look reasonable. The Towns Protocol packages are aligned at ^1.0.2.


9-10: The postgres:18 Docker image tag is valid and available on Docker Hub. No action needed.

drizzle/meta/0009_snapshot.json (4)

62-244: LGTM!

The github_subscriptions table schema correctly reflects the PR objectives:

  • space_id is now nullable (notNull: false)
  • Unique index uses channelId + repoFullName without spaceId

533-624: LGTM!

The oauth_states table correctly has space_id as nullable to support DM channels during OAuth flows.


625-766: LGTM!

The pending_subscriptions table correctly has space_id as nullable, and the unique index properly uses channelId + repoFullName.


418-532: Verify whether DMs should create message mappings and if message_mappings.space_id needs to be nullable.

The review's observation about other tables is correct—github_subscriptions, oauth_states, and pending_subscriptions all have nullable spaceId for DM support. However, message_mappings.space_id is defined as notNull: true and is part of the composite primary key.

All current code paths in message-delivery-service.ts require spaceId when inserting or querying message mappings. No code path found that creates message mappings for DM contexts (where spaceId would be null). If DMs are meant to support message mappings, this would indeed cause insert failures, making the concern valid. If DMs don't need message mappings, the constraint is intentional and correct.

src/services/subscription-service.ts (10)

44-51: LGTM!

The SubscribeParams interface correctly makes spaceId optional and nullable to support DM channels.


229-247: LGTM!

The subscription existence check correctly uses only channelId and repoFullName, with a helpful comment explaining that channelId is globally unique in Towns.


250-265: LGTM!

The insert correctly handles nullable spaceId with the spaceId ?? null pattern.


503-516: LGTM!

The unsubscribe method correctly removes spaceId from its signature and uses only channelId + repoFullName for the delete operation.


521-555: LGTM!

The getSubscription method correctly uses only channelId + repoFullName for lookups.


560-585: LGTM!

The getChannelSubscriptions method correctly queries by channelId only.


383-396: LGTM!

The removeEventTypes method correctly calls unsubscribe with the updated signature (without spaceId).


930-954: LGTM!

The private requiresInstallationFailure method correctly accepts optional spaceId.


960-988: LGTM!

The private storePendingSubscription method correctly handles nullable spaceId with the spaceId ?? null pattern.


1018-1030: LGTM!

The validateRepoAccessAndGetSubscription method correctly uses the updated getSubscription signature without spaceId.

drizzle/0009_cool_mad_thinker.sql (1)

1-7: LGTM!

The migration correctly handles the schema transition:

  1. Drops indexes before modifying columns to avoid constraint violations.
  2. Makes space_id nullable across all three tables to support DM channels.
  3. Recreates unique indexes using only (channel_id, repo_full_name) since channelId is globally unique in Towns.
src/db/schema.ts (5)

53-53: LGTM!

Making spaceId nullable in oauthStates with a clear inline comment explaining DM channel support.


75-75: LGTM!

Consistent nullable spaceId for githubSubscriptions to support DM channels.


102-106: LGTM!

The unique index correctly uses only (channelId, repoFullName) with a clear comment explaining that channelId is globally unique in Towns, making spaceId unnecessary for uniqueness.


212-212: LGTM!

Nullable spaceId in pendingSubscriptions for DM channel support.


225-229: LGTM!

The unique index for pending subscriptions mirrors the githubSubscriptions index pattern, using only (channelId, repoFullName).

tests/unit/handlers/github-subscription-handler.test.ts (4)

520-525: LGTM!

The test correctly expects removeEventTypes to be called with (userId, channelId, repoFullName, eventTypes) without spaceId, matching the updated service signature.


553-553: LGTM!

Index adjusted from [0][3] to [0][2] to correctly access repoFullName after spaceId removal from the call signature.


604-604: LGTM!

Consistent index adjustment for the markdown stripping test.


788-788: LGTM!

Index adjusted from [0][5] to [0][4] to correctly access branchFilter in updateSubscription calls after spaceId removal.

src/services/github-oauth-service.ts (3)

28-35: LGTM!

The OAuthCallbackResult interface correctly types spaceId as string | null with a clear inline comment explaining the DM channel use case.


105-116: LGTM!

The getAuthorizationUrl method signature and JSDoc are updated consistently to accept nullable spaceId for DM channel support.


122-131: LGTM!

The database insert correctly handles nullable spaceId with a clarifying comment.

src/handlers/github-subscription-handler.ts (3)

130-130: LGTM!

Correctly updated to use only channelId for getChannelSubscriptions since channelId is globally unique in Towns.


328-329: LGTM!

Correctly updated to use only channelId for getChannelSubscriptions.


537-540: LGTM!

handleStatus correctly simplified to only destructure channelId since spaceId is no longer needed for subscription queries.

AGENTS.md (2)

17-55: Well-documented discriminated union pattern.

The discriminated union based on isDm is clearly documented with proper type narrowing examples. This accurately reflects how TypeScript will narrow spaceId to null or string based on the discriminator.


361-362: LGTM on Hono app guidance.

Clear documentation that bot.start() returns a Hono app with a reference to Hono's best practices for extending routes.

src/routes/oauth-callback.ts (3)

64-110: Logic correctly simplified for DM support.

The condition change from requiring spaceId to only checking redirectData.repo && townsUserId aligns with the PR objective. The comment on line 74 appropriately documents that spaceId may be null for DMs.


114-145: Consistent with subscribe branch changes.

The subscribe-update flow correctly mirrors the pattern from the subscribe branch, removing spaceId from the precondition check.


148-176: Unsubscribe-update flow correctly updated.

The unsubscribe-update branch properly removes spaceId dependency while ensuring required parameters (repo, townsUserId, eventTypes) are validated.

src/utils/oauth-helpers.ts (3)

18-39: Type signature correctly widened.

The spaceId: string | null change allows this function to support DM channels where spaceId is null.


116-183: Token status handler correctly updated.

The handleInvalidOAuthToken function properly passes the nullable spaceId to both sendEditableOAuthPrompt (for NotLinked and Invalid cases) and oauthService.getAuthorizationUrl (for Unknown case). The exhaustive switch pattern is correctly maintained.


59-111: Core OAuth prompt function correctly updated.

The type change propagates correctly through to oauthService.getAuthorizationUrl. The error logging will show null for DM channels, which provides useful debugging context.

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.

Since channelId is globally unique in Towns, spaceId was redundant for
subscription queries. This change:

- Makes spaceId nullable in database schema (oauth_states,
  github_subscriptions, pending_subscriptions)
- Updates unique indexes to use only channelId + repoFullName
- Removes spaceId from function signatures and WHERE clauses
- Simplifies service calls throughout the codebase

DMs now work because spaceId can be null without breaking queries.
Space subscriptions continue to store spaceId for reference.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
AGENTS.md (2)

297-301: Critical: Example uses spaceId without null check.

The ban command example passes e.spaceId to hasAdminPermission and ban without verifying the event is not a DM. With the new DM support, spaceId will be null in DM contexts, which could cause runtime errors or unexpected behavior.

🔒 Proposed fix with DM check
 // Pattern
 bot.onSlashCommand("ban", async (h, e) => {
+  if (e.isDm) {
+    return await h.sendMessage(e.channelId, "Ban command only works in spaces");
+  }
   if (!(await h.hasAdminPermission(e.userId, e.spaceId)))
     return await h.sendMessage(e.channelId, "No perm");
   await h.ban(e.mentions[0].userId, e.spaceId);
 });

253-253: Clarify spaceId requirements for space-only operations.

Line 253 shows createChannel(spaceId, ...) and Line 364 mentions needing spaceId for external interactions. With DM support, clarify that:

  • Some operations (like createChannel, ban, hasAdminPermission) require a non-null spaceId and only work in space contexts
  • Developers should check isDm before calling space-only operations

Consider adding a note about which handler methods require spaceId vs work in both DM and space contexts.

Also applies to: 364-364

🤖 Fix all issues with AI agents
In `@AGENTS.md`:
- Line 361: Wrap the bare URL in the sentence mentioning `bot.start()` in
AGENTS.md with angle brackets so it becomes
<https://hono.dev/docs/guides/best-practices#building-a-larger-application>,
ensuring markdown linting passes; update the line that reads "`bot.start()`
returns a **Hono app**. To extend with additional routes, create a new Hono app
and use `.route('/', app)` per
https://hono.dev/docs/guides/best-practices#building-a-larger-application" to
include the angle-bracketed URL.
♻️ Duplicate comments (1)
CONTRIBUTING.md (1)

63-70: Malformed Markdown table remains unfixed.

This table still has the issues previously identified:

  1. The header separator row (line 64) has an extra column separator creating 4 columns
  2. The pipe character in line 66's shell command (base64 -i key.pem | tr -d '\n') is unescaped and will be interpreted as a column separator

These issues will prevent the table from rendering correctly.

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d8944ca and 04f42ff.

⛔ Files ignored due to path filters (1)
  • bun.lock is excluded by !**/*.lock
📒 Files selected for processing (15)
  • AGENTS.md
  • CONTRIBUTING.md
  • README.md
  • drizzle/0009_nice_eddie_brock.sql
  • drizzle/meta/0009_snapshot.json
  • drizzle/meta/_journal.json
  • package.json
  • src/db/schema.ts
  • src/handlers/github-subscription-handler.ts
  • src/routes/oauth-callback.ts
  • src/services/github-oauth-service.ts
  • src/services/message-delivery-service.ts
  • src/services/subscription-service.ts
  • src/utils/oauth-helpers.ts
  • tests/unit/handlers/github-subscription-handler.test.ts
🚧 Files skipped from review as they are similar to previous changes (6)
  • src/utils/oauth-helpers.ts
  • drizzle/meta/_journal.json
  • src/handlers/github-subscription-handler.ts
  • src/routes/oauth-callback.ts
  • package.json
  • tests/unit/handlers/github-subscription-handler.test.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.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
  • src/services/github-oauth-service.ts
  • src/db/schema.ts
  • src/services/subscription-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
  • src/services/github-oauth-service.ts
  • src/db/schema.ts
  • src/services/subscription-service.ts
AGENTS.md

📄 CodeRabbit inference engine (CLAUDE.md)

Document agent capabilities, responsibilities, and interfaces in AGENTS.md

Files:

  • AGENTS.md
🧠 Learnings (8)
📚 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: All event handlers receive a base payload including userId, spaceId, channelId, eventId, and createdAt - use eventId as threadId/replyId when responding to maintain event threading

Applied to files:

  • src/services/message-delivery-service.ts
  • src/db/schema.ts
  • AGENTS.md
📚 Learning: 2025-11-18T23:35:49.436Z
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:

  • src/db/schema.ts
  • drizzle/meta/0009_snapshot.json
📚 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 : 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:

  • AGENTS.md
📚 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:

  • AGENTS.md
📚 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 : User IDs are hex addresses in format 0x..., not usernames - use them consistently throughout event handling and message sending

Applied to files:

  • AGENTS.md
📚 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 : Slash commands do not trigger onMessage - register slash command handlers using bot.onSlashCommand() instead

Applied to files:

  • AGENTS.md
📚 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 : Define slash commands in src/commands.ts as a const array with name and description properties, then register handlers using bot.onSlashCommand()

Applied to files:

  • AGENTS.md
📚 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 : Include required environment variables: APP_PRIVATE_DATA (bot credentials) and JWT_SECRET (webhook security token)

Applied to files:

  • CONTRIBUTING.md
🧬 Code graph analysis (1)
src/services/subscription-service.ts (1)
src/db/schema.ts (1)
  • githubSubscriptions (71-110)
🪛 markdownlint-cli2 (0.18.1)
AGENTS.md

361-361: Bare URL used

(MD034, no-bare-urls)

🔇 Additional comments (30)
CONTRIBUTING.md (1)

27-28: LGTM!

The documentation update correctly reflects the new Bun CLI commands for database management. The commands are clear and the comments accurately describe their behavior.

README.md (1)

164-176: LGTM! Improved developer experience with simplified database commands.

The updated instructions are clearer and provide a better developer experience. Using bun db:up and bun db:down abstracts away Docker complexity while the note about named volume persistence helpfully explains data lifecycle during local development.

drizzle/meta/0009_snapshot.json (1)

1-911: LGTM - Auto-generated Drizzle snapshot correctly reflects schema changes.

The snapshot properly captures:

  • Nullable space_id columns across oauth_states, github_subscriptions, pending_subscriptions, and message_mappings
  • Updated composite primary key for message_mappings excluding space_id
  • Recreated unique indexes using only (channel_id, repo_full_name)
src/services/message-delivery-service.ts (5)

37-44: LGTM - DeliveryParams correctly supports nullable spaceId for DM channels.

The type change from string to string | null properly supports DM channels where spaceId is not available.


199-205: LGTM - handleEdit signature updated for nullable spaceId.


274-281: LGTM - handleCreate signature updated for nullable spaceId.


333-372: LGTM - storeMapping correctly handles nullable spaceId with proper conflict resolution.

The onConflictDoUpdate target correctly matches the new composite primary key (channelId, repoFullName, githubEntityType, githubEntityId), and spaceId is properly included in the set clause to update it on conflict since it's no longer part of the uniqueness constraint.


310-331: LGTM - getMessageId correctly queries without spaceId.

The lookup now uses only the globally unique channelId combined with repoFullName and entity identifiers, which aligns with the schema changes where spaceId is removed from the primary key.

src/services/github-oauth-service.ts (3)

28-35: LGTM - OAuthCallbackResult uses undefined for optional spaceId.

Using undefined in the API layer while the DB uses null is a sensible pattern that aligns with TypeScript conventions for optional properties.


110-140: LGTM - getAuthorizationUrl correctly handles undefined-to-null conversion.

The spaceId ?? null conversion on line 126 properly translates the TypeScript undefined to SQL null for database storage.


236-244: LGTM - handleCallback correctly converts null back to undefined.

The stateData.spaceId ?? undefined on line 240 properly converts the database null back to TypeScript undefined for the API response, maintaining consistency with the OAuthCallbackResult interface.

src/db/schema.ts (4)

53-53: LGTM - oauthStates.spaceId made nullable with clear documentation.


74-110: LGTM - githubSubscriptions schema correctly updated.

The nullable spaceId and updated unique index on (channelId, repoFullName) properly support DM channels. The inline comment on line 102 clearly explains the rationale.


203-231: LGTM - pendingSubscriptions schema correctly updated.

Consistent with githubSubscriptions: nullable spaceId and unique index on (channelId, repoFullName) with explanatory comment.


237-279: LGTM - messageMappings schema correctly updated with new primary key.

The composite primary key now correctly excludes spaceId, using only (channelId, repoFullName, githubEntityType, githubEntityId). This aligns with the service layer changes in message-delivery-service.ts where onConflictDoUpdate targets these same columns.

drizzle/0009_nice_eddie_brock.sql (1)

1-10: Verify no duplicate (channel_id, repo_full_name) pairs exist across all spaces before running this migration.

The migration order is correct—it drops indexes before altering columns, drops the old PK, makes columns nullable, then adds the new constraint. However, the migration fundamentally changes the data model from per-space uniqueness to global uniqueness.

Original constraint: (space_id, channel_id, repo_full_name) was unique within each space.
New constraint: (channel_id, repo_full_name) is now globally unique across all spaces.

If existing data contains duplicate (channel_id, repo_full_name) pairs in different spaces, the migration will fail on line 8 when creating the new PK. Before running in production, execute these checks:

Pre-migration verification queries
-- Check github_subscriptions for duplicates across spaces
SELECT channel_id, repo_full_name, COUNT(*) as count, COUNT(DISTINCT space_id) as space_count 
FROM github_subscriptions 
GROUP BY channel_id, repo_full_name 
HAVING COUNT(*) > 1;

-- Check pending_subscriptions for duplicates across spaces
SELECT channel_id, repo_full_name, COUNT(*) as count, COUNT(DISTINCT space_id) as space_count 
FROM pending_subscriptions 
GROUP BY channel_id, repo_full_name 
HAVING COUNT(*) > 1;

-- Check message_mappings for duplicates on new PK fields
SELECT channel_id, repo_full_name, github_entity_type, github_entity_id, COUNT(*) as count, COUNT(DISTINCT space_id) as space_count 
FROM message_mappings 
GROUP BY channel_id, repo_full_name, github_entity_type, github_entity_id 
HAVING COUNT(*) > 1;

If any rows are returned, those duplicates must be resolved before the migration can proceed.

src/services/subscription-service.ts (11)

44-51: LGTM!

The SubscribeParams interface correctly makes spaceId optional with a clear comment explaining that DM channels will have undefined. This aligns with the channelId-centric model where channelId is globally unique.


249-265: LGTM!

The subscription creation correctly:

  1. Converts spaceId from undefined (API convention) to null (DB convention) using spaceId ?? null
  2. Uses channelId + repoFullName for the uniqueness check, matching the database unique index

321-335: LGTM!

The update query correctly uses only channelId and repoFullName in the WHERE clause, consistent with the channelId-centric uniqueness model.


383-411: LGTM!

The removeEventTypes method correctly updates the unsubscribe call to use the new signature and maintains consistency in the WHERE clause logic.


462-468: LGTM!

The conversion sub.spaceId ?? undefined correctly handles the DB-to-API convention, converting null (stored in DB) back to undefined (expected by SubscribeParams.spaceId).


503-516: LGTM!

The unsubscribe method signature correctly removes spaceId, as channelId alone provides sufficient uniqueness for subscription identification.


521-555: LGTM!

The getSubscription method correctly simplifies to use only channelId and repoFullName as parameters, aligning with the globally unique channelId model.


560-585: LGTM!

The getChannelSubscriptions method correctly simplifies to use only channelId for filtering, which is sufficient given the globally unique channel IDs.


595-628: LGTM!

The getRepoSubscribers method correctly returns spaceId: string | null to indicate that DM channels will have null for spaceId. This allows consumers to handle DM vs space channel delivery appropriately.


960-988: LGTM!

The storePendingSubscription method correctly handles optional spaceId with the same params.spaceId ?? null pattern used elsewhere for DB storage.


1018-1023: LGTM!

The getSubscription call correctly uses the updated signature with only channelId and repoFullName.

AGENTS.md (3)

17-39: LGTM! Well-structured discriminated union.

The discriminated union pattern correctly models DM vs Space channels using the isDm discriminator. The type definition enables proper TypeScript narrowing and clearly documents the relationship between isDm and spaceId nullability.


41-54: Excellent type narrowing documentation.

The example clearly demonstrates how TypeScript narrows the BasePayload type based on the isDm discriminator. This will help developers avoid runtime null errors when accessing spaceId.


113-119: Clarify the status of paid commands documentation.

The paid: { price: '$0.20' } syntax shown in the paid commands example (lines 113-119) is not present anywhere in the actual codebase. The command definitions in src/commands.ts currently only support name and description properties. This documentation appears to describe a feature that hasn't been implemented yet. Either:

  • Mark this section as planned/aspirational functionality
  • Remove it until the feature is available
  • Add implementation examples if this is an actual supported feature

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.

@shuhuiluo shuhuiluo merged commit ad0b04b into main Jan 15, 2026
2 checks passed
@shuhuiluo shuhuiluo deleted the feat/dm-support branch January 15, 2026 03:30
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.

2 participants