feat: add /github disconnect command and polish codebase

[shuhuiluo]

• Add /github disconnect command (cascades to remove subscriptions)
• Merge OAuthCleanupService into GitHubOAuthService
• Extract common token fields to reduce duplication
• Use formatSubscriptionSuccess consistently for webhook upgrades
• Add OAUTH_STATE_CLEANUP_INTERVAL_MS constant
• Rename startPeriodicCleanup to startOAuthStateCleanup
• Update README with disconnect, branch filter, and review_comments

🤖 Generated with Claude Code

Summary by CodeRabbit

Release Notes

• New Features

  • Added branch filtering capability to filter subscriptions by branch patterns.
  • Added GitHub account disconnect command for easier account management.
  • Unsubscribe command now supports event-specific filtering via --events parameter.
  • Automatic OAuth state cleanup enabled for improved account security.
  • Enhanced event types support to include review comments.

• Documentation

  • Updated documentation to cover branch filtering usage and delivery modes.

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

[coderabbitai]

Walkthrough

The PR consolidates OAuth state cleanup functionality from a separate OAuthCleanupService into GitHubOAuthService, adds a new "disconnect" command to unlink GitHub accounts, and refactors subscription message handling to use precomputed event types and formatted success messages. A new constant defines the cleanup interval, token storage logic is simplified through field consolidation, and OAuthCleanupService is removed in favor of the integrated approach. Documentation updates reflect branch filtering capabilities.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Key areas requiring attention:

• Verify that cleanup integration works correctly with the moved functionality from OAuthCleanupService to GitHubOAuthService
• Confirm deleteOAuthState() is called in all appropriate places (replacing direct DB deletes)
• Validate that subscription message formatting consistently uses parsed eventTypes and branchFilter throughout all subscription flows
• Test the new "disconnect" command path to ensure it doesn't interfere with existing subscription logic
• Check that token storage consolidation via tokenFields spread doesn't introduce field ordering issues in database operations

Possibly related PRs

• feat: add selective event type subscriptions #21: Continues selective event-type subscription work—both PRs refactor eventTypes handling, propagation, and message formatting in subscription-service and handlers.
• feat: embed GitHub URLs in repo names to reduce message lines #49: Related to subscription success message formatting changes in github-subscription-handler and OAuth callback paths.
• feat: add branch-specific event filtering #70: Adds branch-specific filtering feature (branchFilter column and propagation)—complements this PR's subscription refactoring.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 66.67% 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 title accurately reflects the main changes: introducing the /github disconnect command and general codebase improvements (consolidating cleanup service, reducing duplication, adding constants).

✨ 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/disconnect-and-polish

---

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

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

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

568-586: Consider tracking the timer for graceful shutdown.

The implementation is correct - fire-and-forget initial cleanup with proper error handling, and returning the Timeout for potential cancellation.

For production robustness, consider ensuring the caller (likely src/index.ts) stores this timer and clears it on process shutdown signals (SIGTERM/SIGINT) to prevent orphaned callbacks during graceful shutdown.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 719b160 and 653ac24.

📒 Files selected for processing (7)

• README.md (3 hunks)
• src/constants.ts (1 hunks)
• src/handlers/github-subscription-handler.ts (3 hunks)
• src/index.ts (2 hunks)
• src/services/github-oauth-service.ts (5 hunks)
• src/services/oauth-cleanup-service.ts (0 hunks)
• src/services/subscription-service.ts (3 hunks)

💤 Files with no reviewable changes (1)

• src/services/oauth-cleanup-service.ts

🧰 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/index.ts
• src/handlers/github-subscription-handler.ts
• src/constants.ts
• src/services/github-oauth-service.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/index.ts
• src/handlers/github-subscription-handler.ts
• src/constants.ts
• src/services/github-oauth-service.ts
• src/services/subscription-service.ts

🧬 Code graph analysis (3)

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

src/types/bot.ts (1)

• SlashCommandEvent (13-15)

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

• GitHubOAuthService (68-587)

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

src/db/index.ts (1)

• db (57-57)

src/db/schema.ts (2)

• githubUserTokens (20-42)
• oauthStates (47-65)

src/constants.ts (1)

• OAUTH_STATE_CLEANUP_INTERVAL_MS (76-76)

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

src/formatters/subscription-messages.ts (2)

• formatSubscriptionSuccess (28-40)
• formatDeliveryInfo (7-14)

src/db/schema.ts (1)

• githubSubscriptions (71-110)

🪛 ESLint

src/services/github-oauth-service.ts

[error] 9-9: Unable to resolve path to module 'drizzle-orm'.

(import-x/no-unresolved)

🔇 Additional comments (17)

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

823-844: LGTM! Proper propagation of subscription fields for webhook upgrade messaging.

The changes correctly:

1. Include eventTypes and branchFilter in the RETURNING clause
2. Parse the stored eventTypes string before formatting
3. Use formatSubscriptionSuccess for consistent message rendering

This ensures webhook upgrade notifications display accurate subscription details.

---

466-485: LGTM! Improved consistency with formatSubscriptionSuccess.

The refactor correctly parses event types before passing to createSubscription and uses the shared formatSubscriptionSuccess formatter for consistent messaging.

One note: branchFilter is hardcoded to null on line 482. If pending subscriptions should preserve the original branch filter from when the user initiated the subscription, consider storing and retrieving it from the pendingSubscriptions table.

#!/bin/bash
# Check if pendingSubscriptions schema includes branchFilter column
rg -n "pendingSubscriptions" --type=ts -A 20 src/db/schema.ts | head -40

src/constants.ts (2)

71-76: LGTM! New constant for OAuth state cleanup interval.

The constant is well-documented and the 1-hour interval aligns with the PENDING_SUBSCRIPTION_CLEANUP_INTERVAL_MS cadence, providing consistent periodic cleanup behavior.

---

72-76: Fix incorrect JSDoc: comment says "monthly" but interval is hourly.

The constant value is 60 * 60 * 1000 (1 hour), but the comment incorrectly states "monthly cleanup interval". This should say "hourly" to match the actual behavior.

 /**
- * OAuth state cleanup interval (1 hour)
- * How often to remove expired OAuth state entries from the database.
+ * OAuth state cleanup interval (1 hour)
+ * How often to remove expired OAuth state entries from the database.
  */
 export const OAUTH_STATE_CLEANUP_INTERVAL_MS = 60 * 60 * 1000;

Actually, the comment on line 73 says "(1 hour)" which is correct, but the AI summary mentioned "monthly" which appears to be an error in the summary, not the code. Let me re-read...

The comment correctly states "(1 hour)" on line 73. The code is correct.

README.md (3)

36-36: LGTM! Feature list updated with Branch Filtering.

---

51-65: LGTM! Good documentation for new commands.

The examples clearly show:

• Branch filter syntax with patterns
• The new disconnect command
• Unsubscribe with --events flag

This matches the implementation in the handler.

---

69-73: LGTM! Helpful clarification on branch filtering scope.

The note explaining which events are branch-specific vs. non-branch-specific is valuable for users understanding the filtering behavior.

src/index.ts (2)

134-136: LGTM! Help text updated with new commands.

The help message now includes:

• /github unsubscribe with optional --events flag
• /github disconnect command

This aligns with the handler implementation.

---

228-230: LGTM! OAuth cleanup consolidated into GitHubOAuthService.

Good refactor - removing the separate OAuthCleanupService and calling startOAuthStateCleanup() directly on the OAuth service simplifies the architecture and keeps related functionality together.

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

38-39: LGTM! Usage text updated with disconnect command.

---

66-68: LGTM! Disconnect command routing added.

The switch case correctly routes to the new handleDisconnect function.

---

571-592: LGTM! Clean implementation of disconnect handler.

The implementation correctly:

1. Checks if a token exists before attempting disconnect
2. Provides informative feedback for both cases
3. Displays the GitHub login in the success message for user confirmation

The cascade delete of subscriptions is handled at the database level via the foreign key constraint (onDelete: "cascade" on createdByTownsUserId), which aligns with the PR objective.

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

9-14: LGTM!

The imports are correct. The lt operator from drizzle-orm is the proper function for less-than comparisons in the cleanup query. The ESLint error about module resolution is a configuration issue (likely missing or misconfigured tsconfig paths in ESLint), not a code problem.

---

165-169: LGTM!

Good refactoring to use the encapsulated helper method for OAuth state deletion.

---

198-226: LGTM - Clean DRY refactoring!

The tokenFields extraction correctly consolidates fields that should be set on both insert and upsert, while keeping createdAt separate (insert-only) and townsUserId as the primary key target. This reduces duplication and improves maintainability.

---

392-399: LGTM!

Clean encapsulation of OAuth state deletion that promotes consistency across the class.

---

537-560: LGTM!

The cleanup logic correctly uses lt() to find expired states and efficiently returns the count via .returning(). Error handling appropriately logs and re-throws for caller awareness.