fix: Correctly cleanup existing slack integrations

[johnjcsmith]

User description

• Avoid circular dependency by using drizzle directly to cleanup conflicting Slack integrations.
• Handle duplicate Nango connections for the same cluster

---

PR Type

Bug fix, Enhancement

---

Description

• Fixed circular dependency issue in Slack integration cleanup by removing dependency on upsertIntegrations
• Improved conflict resolution by directly using database operations instead of recursive calls
• Enhanced logging for better tracking of Slack integration conflicts and cleanup
• Simplified connection management by extracting cleanup logic into a dedicated function
• Improved code organization and variable naming for better maintainability

---

Changes walkthrough 📝

	Relevant files
Enhancement	integrations.ts Rename and refactor installable integrations variable        control-plane/src/modules/integrations/integrations.ts Renamed installableIntegrations variable to installables for better readability Updated references to use the new variable name throughout the file +4/-4
Bug fix	index.ts Improve Slack integration cleanup and conflict resolution control-plane/src/modules/integrations/slack/index.ts Extracted Slack integration cleanup logic into a new cleanupConflictingConnections function Removed direct dependency on upsertIntegrations Simplified deleteNangoConnection by removing clusterId parameter Added better logging for conflict resolution +51/-32

---

💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

[qodo-merge-pro]

Qodo Merge was enabled for this repository. To continue using it, please link your Git account with your Qodo account here.

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Error Handling The new cleanupConflictingConnections function should handle potential errors during Nango connection deletion and database cleanup operations to ensure partial failures don't leave the system in an inconsistent state. await Promise.all(conflicts.map((conflict) => { if (conflict.slack) { logger.info("Removing conflicting Slack Nango connections", { clusterId: conflict.cluster_id, connectionId: conflict.slack.nangoConnectionId }) return deleteNangoConnection(conflict.slack.nangoConnectionId); } })); if (conflicts.length) { logger.info("Removed conflicting Slack integrations from DB", { conflicts: conflicts.map((conflict) => conflict.cluster_id) }) // Cleanup Slack integrations await db .delete(integrations) .where( and( sql`slack->>'teamId' = ${config.slack.teamId}`, ne(integrations.cluster_id, clusterId) ) ); } Race Condition The gap between cleaning up Nango connections and database records could lead to race conditions if multiple cleanup operations happen simultaneously. Consider using a transaction. await Promise.all(conflicts.map((conflict) => { if (conflict.slack) { logger.info("Removing conflicting Slack Nango connections", { clusterId: conflict.cluster_id, connectionId: conflict.slack.nangoConnectionId }) return deleteNangoConnection(conflict.slack.nangoConnectionId); } })); if (conflicts.length) { logger.info("Removed conflicting Slack integrations from DB", { conflicts: conflicts.map((conflict) => conflict.cluster_id) }) // Cleanup Slack integrations await db .delete(integrations) .where( and( sql`slack->>'teamId' = ${config.slack.teamId}`, ne(integrations.cluster_id, clusterId) ) );

[qodo-merge-pro]

Qodo Merge was enabled for this repository. To continue using it, please link your Git account with your Qodo account here.

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Score
Possible issue	Wrap cleanup operations in a database transaction to maintain data consistency Add a transaction around the cleanup operations to ensure atomicity between Nango connection deletions and database updates. This prevents inconsistent states. control-plane/src/modules/integrations/slack/index.ts [296-322] -// Cleanup Nango connections -await Promise.all(conflicts.map((conflict) => { - if (conflict.slack) { - logger.info("Removing conflicting Slack Nango connections", { - clusterId: conflict.cluster_id, - connectionId: conflict.slack.nangoConnectionId - }) - return deleteNangoConnection(conflict.slack.nangoConnectionId); +await db.transaction(async (trx) => { + // Cleanup Nango connections + await Promise.allSettled(conflicts.map(async (conflict) => { + if (conflict.slack) { + try { + logger.info("Removing conflicting Slack Nango connections", { + clusterId: conflict.cluster_id, + connectionId: conflict.slack.nangoConnectionId + }); + await deleteNangoConnection(conflict.slack.nangoConnectionId); + } catch (error) { + logger.error("Failed to delete Nango connection", { error }); + throw error; + } + } + })); + + if (conflicts.length) { + logger.info("Removing conflicting Slack integrations from DB", { + conflicts: conflicts.map((conflict) => conflict.cluster_id) + }); + + // Cleanup Slack integrations + await trx + .delete(integrations) + .where( + and( + sql`slack->>'teamId' = ${config.slack.teamId}`, + ne(integrations.cluster_id, clusterId) + ) + ); } -})); +}); -if (conflicts.length) { - logger.info("Removed conflicting Slack integrations from DB", { - conflicts: conflicts.map((conflict) => conflict.cluster_id) - }) - - // Cleanup Slack integrations - await db - .delete(integrations) - .where( - and( - sql`slack->>'teamId' = ${config.slack.teamId}`, - ne(integrations.cluster_id, clusterId) - ) - ); -} - Apply this suggestion Suggestion importance[1-10]: 9 Why: The suggestion adds crucial transaction handling to ensure atomicity between Nango connection deletions and database updates, preventing data inconsistencies that could occur if one operation fails. This is essential for data integrity.	9
	✅ Use Promise.allSettled with proper error handling to ensure robust cleanup of multiple connections Add error handling for the Promise.all operation in cleanupConflictingConnections to ensure the function continues even if some connection deletions fail. This prevents partial cleanup scenarios. control-plane/src/modules/integrations/slack/index.ts [296-304] -await Promise.all(conflicts.map((conflict) => { +await Promise.allSettled(conflicts.map(async (conflict) => { if (conflict.slack) { - logger.info("Removing conflicting Slack Nango connections", { - clusterId: conflict.cluster_id, - connectionId: conflict.slack.nangoConnectionId - }) - return deleteNangoConnection(conflict.slack.nangoConnectionId); + try { + logger.info("Removing conflicting Slack Nango connections", { + clusterId: conflict.cluster_id, + connectionId: conflict.slack.nangoConnectionId + }); + await deleteNangoConnection(conflict.slack.nangoConnectionId); + } catch (error) { + logger.error("Failed to delete Nango connection", { + error, + clusterId: conflict.cluster_id, + connectionId: conflict.slack.nangoConnectionId + }); + } } })); [Suggestion has been applied] Suggestion importance[1-10]: 8 Why: The suggestion significantly improves error handling by using Promise.allSettled and try-catch blocks, preventing the cleanup process from failing entirely if a single connection deletion fails. This is crucial for system reliability.	8