feat: implement pending subscriptions for one-click private repo setup

[shuhuiluo]

Enables automatic subscription completion after GitHub App installation, eliminating the need for users to run the subscribe command twice.

When a user tries to subscribe to a private repo without GitHub App installed:

1. Subscription is stored as pending in the database
2. User is prompted to install the GitHub App
3. Installation webhook automatically completes the pending subscription
4. Success message is sent to Towns channel

Features:

• Add pendingSubscriptions table with unique constraint on (channel_id, repo_full_name)
• Store pending subscription when private repo requires installation
• Auto-complete pending subscriptions on installation webhook
• Add periodic cleanup task for expired pending subscriptions (1 hour TTL)
• Handle duplicate pending subscriptions atomically with onConflictDoNothing
• Update InstallationService to trigger pending subscription completion

Database migrations:

• 0003_wooden_romulus.sql: Create pendingSubscriptions table
• 0004_sticky_black_tarantula.sql: Add unique index on (channel_id, repo_full_name)

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features
  • Added pending subscription workflow: subscription requests for private repositories without the required GitHub App installation are now automatically stored and completed once installation is complete.
  • Automatic cleanup process removes expired pending subscriptions.
  • Improved user experience by preserving subscription requests during the GitHub App installation process.

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

[coderabbitai]

Walkthrough

This PR introduces a pending subscriptions feature to defer GitHub repository subscription activation when the required GitHub App installation is unavailable. The changes add a new database table pending_subscriptions with associated migration and schema definitions, introduce expiration and cleanup constants, implement storage logic for pending subscriptions in the subscription service, add a completion workflow triggered after installation, and integrate automatic completion into the installation upgrade flow. Expired pending subscriptions are periodically cleaned up via scheduled intervals.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20–25 minutes

• src/services/subscription-service.ts: Review the new storePendingSubscription, completePendingSubscriptions, and cleanupExpiredPendingSubscriptions methods for correctness of database operations, error handling, and OAuth token validation logic during completion.
• src/github-app/installation-service.ts: Verify the call to completePendingSubscriptions(repoFullName) integrates correctly and that logging is appropriate.
• Database schema and migration files: Confirm the table structure, indexes, foreign key constraints, and uniqueness constraints match the service logic assumptions.
• Cleanup interval scheduling: Validate that the periodic cleanup interval is correctly set up in the constructor and that expired subscription deletion works as intended.

Possibly related PRs

• refactor: extract duplicate repo addition logic in InstallationService #48: Modifies the same addRepoAndUpgrade method in src/github-app/installation-service.ts that this PR extends to complete pending subscriptions.
• feat: implement OAuth-first subscription service with dual-mode delivery #32: Modifies subscription and installation workflow in both subscription-service.ts and installation-service.ts, directly related to the pending subscription integration points.
• fix: enable private repo subscriptions via OAuth token #57: Updates the subscription flow in src/services/subscription-service.ts to handle cases where a repo requires GitHub App installation, providing context for where pending subscriptions are stored.

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The pull request title 'feat: implement pending subscriptions for one-click private repo setup' clearly and specifically describes the main change—implementing pending subscriptions to enable one-click setup for private repositories.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ 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/pending-subscriptions

---

📜 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 3acc851 and 99bca45.

📒 Files selected for processing (7)

• drizzle/0003_wet_beyonder.sql (1 hunks)
• drizzle/meta/0003_snapshot.json (1 hunks)
• drizzle/meta/_journal.json (1 hunks)
• src/constants.ts (1 hunks)
• src/db/schema.ts (1 hunks)
• src/github-app/installation-service.ts (2 hunks)
• src/services/subscription-service.ts (7 hunks)

🚧 Files skipped from review as they are similar to previous changes (4)

• src/github-app/installation-service.ts
• drizzle/meta/0003_snapshot.json
• src/constants.ts
• src/db/schema.ts

🧰 Additional context used

🧠 Learnings (1)

📚 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/services/subscription-service.ts

🧬 Code graph analysis (1)

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

src/constants.ts (2)

• PENDING_SUBSCRIPTION_CLEANUP_INTERVAL_MS (51-51)
• PENDING_SUBSCRIPTION_EXPIRATION_MS (45-45)

src/db/index.ts (1)

• db (57-57)

src/db/schema.ts (1)

• pendingSubscriptions (200-228)

🔇 Additional comments (9)

drizzle/0003_wet_beyonder.sql (1)

1-16: LGTM! Well-structured migration with appropriate indexes and constraints.

The table design is solid:

• Proper use of CASCADE delete for the foreign key (appropriate for temporary pending data)
• Comprehensive index coverage for cleanup queries (expires_at), user lookups, and repo-based completion
• Unique constraint on (space_id, channel_id, repo_full_name) prevents duplicate pending subscriptions

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

1-1: LGTM! Imports are correctly added.

The new imports support the pending subscriptions feature:

• sql for raw SQL in cleanup queries
• PENDING_SUBSCRIPTION_* constants for expiration and scheduling
• pendingSubscriptions table reference for database operations

Also applies to: 14-15, 18-18

---

92-95: LGTM! Cleanup interval properly scheduled.

The periodic cleanup runs every hour to remove expired pending subscriptions. Using void to ignore the promise is appropriate here.

---

160-167: LGTM! Correctly stores pending subscription for potential private repo.

When encountering a 404 without installation, storing the pending subscription enables one-click completion after the user installs the GitHub App.

---

207-214: LGTM! Correctly stores pending subscription for private repo requiring installation.

Storing the pending subscription here ensures automatic completion once the user installs the GitHub App on the private repository.

---

292-320: LGTM! Robust implementation with atomic duplicate handling.

The method correctly:

• Sets a 1-hour expiration for pending subscriptions
• Uses onConflictDoNothing() to atomically handle duplicate pending subscriptions (leveraging the unique index)
• Provides appropriate logging for debugging

---

327-388: LGTM! Completion logic correctly handles pending subscriptions.

The implementation appropriately:

• Validates OAuth tokens before attempting subscription creation
• Sends success notifications to channels
• Removes all pending entries after processing (consistent with past review discussion—no retry path exists after installation webhook)
• Handles errors gracefully with logging and continues processing other subscriptions

---

716-734: LGTM! Cleanup logic is well-implemented.

The method correctly:

• Uses server-side NOW() comparison to avoid timezone issues
• Provides appropriate logging only when entries are removed
• Handles errors gracefully without crashing the background task

drizzle/meta/_journal.json (1)

26-32: LGTM! Standard migration journal update.

The new entry correctly records the migration metadata in the expected Drizzle format.

---

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

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (3)

src/constants.ts (1)

41-51: Consider more frequent cleanup interval.

The cleanup interval matches the expiration time (both 1 hour), which means expired pending subscriptions could persist for up to 2 hours before removal. For comparison, PENDING_MESSAGE_CLEANUP_INTERVAL_MS runs twice as frequently (30s) as PENDING_MESSAGE_MAX_AGE_MS (60s).

Consider setting PENDING_SUBSCRIPTION_CLEANUP_INTERVAL_MS to 30 minutes (30 * 60 * 1000) to clean up expired entries more promptly.

 /**
  * Pending subscription cleanup interval (1 hour)
  * How often to check for and remove expired pending subscriptions
  */
-export const PENDING_SUBSCRIPTION_CLEANUP_INTERVAL_MS = 60 * 60 * 1000;
+export const PENDING_SUBSCRIPTION_CLEANUP_INTERVAL_MS = 30 * 60 * 1000; // 30 minutes

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

92-96: Interval-based cleanup is fine; consider lifecycle management if multiple instances exist

The additional setInterval for cleanupExpiredPendingSubscriptions is reasonable, but if SubscriptionService is ever instantiated more than once per process, each instance will schedule its own cleanup loop hitting the DB. If this service might be non-singleton, consider centralizing interval scheduling or keeping the timer handle on the instance with a dispose() to clear it when no longer needed.

---

160-167: Pending subscription storage matches the flow; be aware of re-subscribe/unsubscribe edge cases

Storing a single pending row per (channelId, repoFullName) via .onConflictDoNothing() is a good way to dedupe repeated /subscribe calls while the app isn’t installed. One behavioral edge case to be aware of: if a user changes their mind (e.g., runs /github unsubscribe or re-subscribes with different eventTypes) before installation completes, the existing pending record will still be used and auto-subscribed with the original eventTypes. If that’s undesirable, you may want to (in a follow-up) either:

• Clear any pending row from pendingSubscriptions when handling unsubscribe, and/or
• Allow updating eventTypes on an existing pending row instead of ignoring later requests.

Not blocking, but worth confirming this matches intended UX.

Also applies to: 207-214

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between aea6136 and 3acc851.

📒 Files selected for processing (9)

• drizzle/0003_wooden_romulus.sql (1 hunks)
• drizzle/0004_sticky_black_tarantula.sql (1 hunks)
• drizzle/meta/0003_snapshot.json (1 hunks)
• drizzle/meta/0004_snapshot.json (1 hunks)
• drizzle/meta/_journal.json (1 hunks)
• src/constants.ts (1 hunks)
• src/db/schema.ts (1 hunks)
• src/github-app/installation-service.ts (2 hunks)
• src/services/subscription-service.ts (7 hunks)

🧰 Additional context used

🧠 Learnings (2)

📓 Common learnings

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.

📚 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

🧬 Code graph analysis (1)

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

src/constants.ts (2)

• PENDING_SUBSCRIPTION_CLEANUP_INTERVAL_MS (51-51)
• PENDING_SUBSCRIPTION_EXPIRATION_MS (45-45)

src/db/index.ts (1)

• db (57-57)

src/db/schema.ts (1)

• pendingSubscriptions (200-227)

🔇 Additional comments (5)

src/github-app/installation-service.ts (1)

272-292: LGTM! Clean integration of pending subscription completion.

The flow correctly completes pending subscriptions after upgrading existing subscriptions to webhook mode. Error handling and logging are consistent with existing patterns.

drizzle/0003_wooden_romulus.sql (1)

1-15: LGTM! Well-structured migration with appropriate constraints.

The table structure, foreign key cascade behavior, and indexes are well-designed:

• Cascade delete on towns_user_id is correct since pending subscriptions are tied to user lifecycle
• Indexes support the expected query patterns (cleanup by expiration, lookup by user/repo)

src/db/schema.ts (1)

195-227: LGTM! Schema definition correctly matches SQL migrations.

The Drizzle schema accurately reflects the database structure created by migrations 0003 and 0004, with appropriate foreign key cascade behavior and indexes.

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

288-321: Pending subscription persistence looks correct and idempotent

storePendingSubscription correctly:

• Derives expiresAt from PENDING_SUBSCRIPTION_EXPIRATION_MS.
• Inserts a single row tied to townsUserId (leveraging the FK for referential integrity).
• Uses .onConflictDoNothing() so concurrent or repeated /subscribe calls for the same (channelId, repoFullName) don’t create duplicates.

No changes needed here.

---

716-734: Expiry cleanup logic is sound; uses DB time and expiry index

The cleanupExpiredPendingSubscriptions implementation looks good:

• It relies on NOW() in the database, which avoids skew with the app server clock.
• It leverages the expiresAt column (with an index in the schema) as the filter.
• Errors are caught and logged so the interval doesn’t crash on exception.

No functional changes required here.