Feat: Allow users to remove one than 1 connection #398
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,42 +12,121 @@ import { type DocumentDBClusterItem } from '../../tree/connections-view/Document | |
| import { getConfirmationAsInSettings } from '../../utils/dialogs/getConfirmation'; | ||
| import { showConfirmationAsInSettings } from '../../utils/dialogs/showConfirmation'; | ||
|
|
||
| export async function removeConnection( | ||
| context: IActionContext, | ||
| node?: DocumentDBClusterItem, | ||
| nodes?: DocumentDBClusterItem[], | ||
| ): Promise<void> { | ||
| // Determine the list of connections to delete | ||
| let connectionsToDelete: DocumentDBClusterItem[]; | ||
| if (nodes && nodes.length > 0) { | ||
| connectionsToDelete = nodes; | ||
| } else if (node) { | ||
| connectionsToDelete = [node]; | ||
| } else { | ||
| connectionsToDelete = []; | ||
| } | ||
|
|
||
| if (connectionsToDelete.length === 0) { | ||
| ext.outputChannel.warn(l10n.t('No connections selected to remove.')); | ||
| return; | ||
| } | ||
|
|
||
| // Set telemetry for the first node | ||
| context.telemetry.properties.experience = connectionsToDelete[0].experience.api; | ||
| context.telemetry.measurements.connectionsToDelete = connectionsToDelete.length; | ||
|
|
||
| // Confirmation logic - different messages for single vs. multiple deletions | ||
| const expectedConfirmationWord = | ||
| connectionsToDelete.length === 1 ? 'delete' : connectionsToDelete.length.toString(); | ||
| const confirmed = await getConfirmationAsInSettings( | ||
| l10n.t('Are you sure?'), | ||
| connectionsToDelete.length === 1 | ||
| ? l10n.t('Delete "{connectionName}"?', { connectionName: connectionsToDelete[0].cluster.name }) + | ||
| '\n' + | ||
| l10n.t('This cannot be undone.') | ||
| : l10n.t('Delete {count} connections?', { count: connectionsToDelete.length }) + | ||
| '\n' + | ||
| l10n.t('This cannot be undone.'), | ||
| expectedConfirmationWord, | ||
| ); | ||
|
|
||
| if (!confirmed) { | ||
| throw new UserCancelledError(); | ||
| } | ||
|
|
||
| // Resilient deletion loop - continue on failure | ||
| let successCount = 0; | ||
| let failureCount = 0; | ||
|
|
||
| for (const connection of connectionsToDelete) { | ||
| try { | ||
| await ext.state.showDeleting(connection.id, async () => { | ||
| if (connection.cluster.emulatorConfiguration?.isEmulator) { | ||
| await ConnectionStorageService.delete(ConnectionType.Emulators, connection.storageId); | ||
| } else { | ||
| await ConnectionStorageService.delete(ConnectionType.Clusters, connection.storageId); | ||
| } | ||
| }); | ||
|
|
||
| // delete cached credentials from memory | ||
| CredentialCache.deleteCredentials(connection.id); | ||
|
|
||
| // Log success | ||
| ext.outputChannel.info( | ||
| l10n.t('Successfully removed connection "{connectionName}".', { | ||
| connectionName: connection.cluster.name, | ||
| }), | ||
| ); | ||
| successCount++; | ||
| } catch (error) { | ||
| // Log error and continue with next connection | ||
| ext.outputChannel.error( | ||
| l10n.t('Failed to remove connection "{connectionName}": {error}', { | ||
| connectionName: connection.cluster.name, | ||
| error: error instanceof Error ? error.message : String(error), | ||
| }), | ||
| ); | ||
|
|
||
| // Note: When multiple deletions fail, we intentionally capture only the last error's | ||
| // details in telemetry. The errorCount metric below provides context on total failures. | ||
| context.telemetry.properties.error = 'RemoveConnectionError'; | ||
| context.telemetry.properties.errorMessage = error instanceof Error ? error.message : String(error); | ||
|
Comment on lines
+93
to
+94
There was a problem hiding this comment. When multiple deletions fail, the telemetry properties for Consider either:
Example fix for option 1: let firstError: Error | undefined;
// ... in the catch block:
if (!firstError) {
firstError = error instanceof Error ? error : new Error(String(error));
}
// ... after the loop:
if (firstError) {
context.telemetry.properties.error = 'RemoveConnectionError';
context.telemetry.properties.errorMessage = firstError.message;
}
Collaborator
Author
There was a problem hiding this comment. @copilot, this is totally fine, capturing one is enough, I was aware of it. Please add a comment explaining that we're aware of it. and.. well, add, once when metrics are set, the errorCount as well. This is a derived value, but will add context. |
||
|
|
||
| failureCount++; | ||
| } | ||
| } | ||
|
|
||
| // Refresh the tree view | ||
| ext.connectionsBranchDataProvider.refresh(); | ||
|
|
||
| // Set telemetry for successfully deleted connections | ||
| context.telemetry.measurements.connectionsDeleted = successCount; | ||
| context.telemetry.measurements.errorCount = failureCount; | ||
|
|
||
| // Show summary message | ||
| if (connectionsToDelete.length === 1) { | ||
| // For single connection, show success or error message | ||
| if (successCount === 1) { | ||
| showConfirmationAsInSettings(l10n.t('The selected connection has been removed.')); | ||
| } else { | ||
| // Error is already logged to outputChannel, so throw error to show notification | ||
| throw new Error( | ||
| l10n.t('Failed to remove connection "{connectionName}".', { | ||
| connectionName: connectionsToDelete[0].cluster.name, | ||
| }), | ||
| ); | ||
| } | ||
| } else { | ||
| // Show summary for multiple deletions | ||
| const summaryMessage = | ||
| failureCount === 0 | ||
| ? l10n.t('Successfully removed {count} connections.', { count: successCount }) | ||
| : l10n.t('Removed {successCount} of {total} connections. {failureCount} failed.', { | ||
| successCount, | ||
| total: connectionsToDelete.length, | ||
| failureCount, | ||
| }); | ||
| showConfirmationAsInSettings(summaryMessage); | ||
| } | ||
| } | ||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the function is called with no connections to delete (both
nodeandnodesare undefined or empty), it returns silently without any user feedback. This could leave users confused if the command is invoked unexpectedly without a selection.Consider logging a warning or showing a message to inform the user:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot agreed, please add this line, remember about running 'npm run prettier-fix' and 'npm run l10n' after making the edits.