fix: restore polling mode when GitHub App is uninstalled #56
Conversation
WalkthroughAdds a pre-deletion downgrade flow invoked from InstallationService.onInstallationDeleted and when repositories are removed from an installation. InstallationService now calls a private handleDowngrade(installationId, repos?) which uses SubscriptionService.downgradeSubscriptions. The new method locates subscriptions for the installation (optionally filtered to specific repos), converts public-repo subscriptions to deliveryMode "polling" and clears their installation association, deletes private-repo subscriptions, sends per-subscription notifications with per-message error handling, and returns counts of downgraded and removed subscriptions which are logged. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/services/subscription-service.ts (1)
491-524: Consider parallelizing notifications for better performance.The notification loops currently send messages sequentially, which could be slow when many subscriptions are affected. Consider using
Promise.allSettledfor parallel execution, similar to the approach inupgradeToWebhook(lines 368-391).Apply this refactor for public repo notifications:
- for (const sub of publicRepos) { - try { - await this.bot.sendMessage( - sub.channelId, - `⚠️ GitHub App uninstalled for **${sub.repoFullName}**\n\n` + - `Downgraded to polling mode (5-minute intervals). ` + - `Reinstall the app for real-time webhooks.` - ); - } catch (error) { - console.error( - `Failed to notify channel ${sub.channelId} about downgrade:`, - error - ); - } - } + await Promise.allSettled( + publicRepos.map(async sub => { + try { + await this.bot!.sendMessage( + sub.channelId, + `⚠️ GitHub App uninstalled for **${sub.repoFullName}**\n\n` + + `Downgraded to polling mode (5-minute intervals). ` + + `Reinstall the app for real-time webhooks.` + ); + } catch (error) { + console.error( + `Failed to notify channel ${sub.channelId} about downgrade:`, + error + ); + } + }) + );Apply the same pattern for private repo notifications (lines 509-523).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/github-app/installation-service.ts(1 hunks)src/services/subscription-service.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/services/subscription-service.ts (2)
src/db/index.ts (1)
db(57-57)src/db/schema.ts (1)
githubSubscriptions(72-109)
🔇 Additional comments (5)
src/github-app/installation-service.ts (2)
78-102: LGTM! Well-structured downgrade flow with appropriate error handling.The pre-deletion downgrade logic is well-designed:
- Calls
downgradeToPollingbefore deleting the installation (correct order)- Error handling ensures installation deletion proceeds even if downgrade fails
- Conditional logging avoids noise when counts are zero
- The foreign key
SET NULLconstraint acts as a safety net if this downgrade fails
105-105: Accurate comment clarifying foreign key behavior.The comment correctly documents the
SET NULLcascade behavior from the foreign key constraint, which serves as a safety net if the downgrade flow fails.src/services/subscription-service.ts (3)
426-448: LGTM! Clean query logic with efficient early return.The method correctly retrieves affected subscriptions and handles the no-op case efficiently.
450-489: LGTM! Subscription split and database operations are correct.The logic properly handles both subscription types:
- Public repos: downgraded to polling mode (users can still receive updates)
- Private repos: removed (cannot be polled without app access)
The database operations correctly update/delete based on repository privacy with appropriate WHERE clauses.
526-527: LGTM! Return value accurately reflects operations performed.The counts from
.returning()provide accurate feedback for logging in the caller.
Fixes critical bug where polling stopped entirely after uninstalling the GitHub App. **Problem:** When users uninstalled the GitHub App, subscriptions kept `deliveryMode = "webhook"` but `installationId` became NULL. The polling service only polls repos with `deliveryMode = "polling"`, so these orphaned subscriptions were ignored. **Solution:** - Add `downgradeToPolling()` method to SubscriptionService - Call it from `onInstallationDeleted()` before deleting installation - Public repos: Automatically downgrade from webhook → polling mode - Private repos: Remove subscription (can't poll without app access) - Users are notified in Towns about the change **Implementation Details:** SubscriptionService.downgradeToPolling(): - Finds all subscriptions for the deleted installation - Splits into public (can poll) vs private (requires app) - Public: Updates `deliveryMode = "polling"`, sets `installationId = NULL` - Private: Deletes subscription from database - Sends Towns notifications to affected channels - Returns counts for logging InstallationService.onInstallationDeleted(): - Calls downgradeToPolling() before deleting installation record - Logs downgrade/removal counts - Includes error handling (continues deletion even if downgrade fails) **Result:** Polling resumes automatically for public repos when app is uninstalled. Users see clear notification about the downgrade and how to restore webhooks. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Replace downgradeToPolling() with downgradeSubscriptions() that accepts optional repos filter - Add handleDowngrade() helper to eliminate duplicate try/catch blocks - Parallelize notifications with Promise.allSettled - Fix missing downgrade call in onRepositoriesRemoved 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
shuhuiluo
force-pushed
the
fix/polling-after-app-uninstall
branch
from
58b5e87 to
d802ada
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/services/subscription-service.ts (1)
392-450: Core logic is sound; consider transaction wrapping.The split into public (downgrade) vs. private (remove) repos is correct, and the defensive WHERE conditions provide good protection against race conditions.
Consider wrapping the UPDATE and DELETE operations in a database transaction to ensure atomicity. Currently, if one operation fails, you could have partial state.
Apply this pattern:
// Wrap UPDATE and DELETE in transaction await db.transaction(async (tx) => { // Downgrade public repos if (publicRepos.length > 0) { const result = await tx.update(githubSubscriptions) .set({ deliveryMode: "polling", installationId: null, updatedAt: new Date() }) .where(and(...updateConditions)) .returning({ id: githubSubscriptions.id }); downgraded = result.length; } // Remove private repos if (privateRepos.length > 0) { const result = await tx.delete(githubSubscriptions) .where(and(...deleteConditions)) .returning({ id: githubSubscriptions.id }); removed = result.length; } });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/github-app/installation-service.ts(3 hunks)src/services/subscription-service.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/github-app/installation-service.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/services/subscription-service.ts (2)
src/db/schema.ts (1)
githubSubscriptions(71-108)src/db/index.ts (1)
db(57-57)
🔇 Additional comments (3)
src/services/subscription-service.ts (3)
1-1: LGTM! Import addition is appropriate.The
inArrayimport is correctly used in the newdowngradeSubscriptionsmethod for filtering by repository arrays.
372-390: Well-documented and handles optional filtering correctly.The method signature and documentation clearly explain the dual-purpose design (full installation vs. specific repos). The guard on line 388 correctly prevents passing empty arrays to
inArray.
452-489: Excellent notification handling with proper error recovery.The use of
Promise.allSettledensures all notifications are attempted even if some fail, and the per-notification error handling prevents failures from blocking the downgrade process. The user-facing messages clearly explain what happened and how to restore webhook delivery.
Ensures UPDATE and DELETE operations are atomic. If one fails, both roll back to prevent partial state. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/services/subscription-service.ts (1)
372-493: downgradeSubscriptions logic matches intended behavior; consider guarding empty repo listThe method cleanly:
- Targets subscriptions by
installationId(and optionallyrepos),- Splits public vs private using
isPrivate,- Applies downgrade/delete in a single transaction,
- Then notifies affected channels with per-message error handling.
One edge case worth tightening: if
reposis provided but happens to be an empty array, the currentif (repos && repos.length > 0)check means you’ll treat that as “no repo filter” and downgrade/remove all repos for the installation. If callers ever pass[]by mistake, this could produce a broader downgrade than intended.If you want safer, more explicit behavior, you can early-return on an empty repo list:
async downgradeSubscriptions( installationId: number, repos?: string[] ): Promise<{ downgraded: number; removed: number }> { - // Build WHERE conditions + // No repos specified: treat as a no-op for this call site + if (repos && repos.length === 0) { + return { downgraded: 0, removed: 0 }; + } + + // Build WHERE conditions const conditions = [eq(githubSubscriptions.installationId, installationId)]; if (repos && repos.length > 0) { conditions.push(inArray(githubSubscriptions.repoFullName, repos)); }This keeps the default “all repos” behavior when
reposisundefined, but avoids accidentally mass-downgrading if an empty array sneaks through.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/services/subscription-service.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/services/subscription-service.ts (2)
src/db/schema.ts (1)
githubSubscriptions(71-108)src/db/index.ts (1)
db(57-57)
🔇 Additional comments (1)
src/services/subscription-service.ts (1)
1-1: Import update correctly reflects new query usageThe addition of
inArrayhere matches its usage indowngradeSubscriptionsand keeps the drizzle import set coherent. No further changes needed.
Treat empty array as no-op instead of downgrading all repos for installation. Prevents accidental mass downgrade. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/services/subscription-service.ts (1)
372-498: Strong implementation with clear separation of concerns.The method correctly handles the complex downgrade flow: querying affected subscriptions, splitting by visibility, atomically updating/deleting within a transaction, and notifying channels in parallel. The logic is sound, error handling is comprehensive, and the return values accurately reflect database changes.
Optional enhancement: Consider logging the downgrade/removal counts within this method for observability, even though the caller also logs them:
console.log( `Downgraded ${downgraded} public repo subscription(s) and removed ${removed} private repo subscription(s) for installation ${installationId}` ); return { downgraded, removed };This would provide a complete audit trail at the service layer, independent of how callers choose to log.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/services/subscription-service.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/services/subscription-service.ts (2)
src/db/schema.ts (1)
githubSubscriptions(71-108)src/db/index.ts (1)
db(57-57)
🔇 Additional comments (4)
src/services/subscription-service.ts (4)
386-389: LGTM: Clean edge-case handling.The empty-array check correctly distinguishes between "all repos" (undefined) and "no repos" (empty array), avoiding unnecessary database operations.
420-459: Excellent defensive programming and transaction handling.The redundant
isPrivatechecks in update (line 427) and delete (line 449) conditions provide an additional safety layer beyond the earlier filtering. Combined with the atomic transaction, this ensures data integrity even if the filtering logic were to change or have bugs.
461-495: LGTM: Robust parallel notification handling.Using
Promise.allSettledwith per-message error handling ensures all notifications are attempted independently, and failures are logged without blocking the operation. Sending notifications after the transaction commits is the correct pattern—database changes are atomic while notifications are best-effort.
1-1:inArrayis available and supported in drizzle-orm 0.44.7.Drizzle-orm v0.44.7 supports inArray, confirming the import at line 1 is valid and the filtering logic in
downgradeSubscriptionswill function as intended.
Fixes critical bug where polling stopped entirely after uninstalling the GitHub App.
Problem:
When users uninstalled the GitHub App, subscriptions kept
deliveryMode = "webhook"butinstallationIdbecame NULL. The polling service only polls repos withdeliveryMode = "polling", so these orphaned subscriptions were ignored.Solution:
downgradeToPolling()method to SubscriptionServiceonInstallationDeleted()before deleting installationImplementation Details:
SubscriptionService.downgradeToPolling():
deliveryMode = "polling", setsinstallationId = NULLInstallationService.onInstallationDeleted():
Result:
Polling resumes automatically for public repos when app is uninstalled. Users see clear notification about the downgrade and how to restore webhooks.
🤖 Generated with Claude Code
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.