fix: minor updates and fixes to features released in 0.7.0 #490
Conversation
- Add credential validation at paste wizard start (H-3) - Add pre-flight validation in ExecuteStep before task start (M-1) - Clear stale clipboard reference when source is no longer valid - Show meaningful error messages to users - Add telemetry for tracking validation failures
- Add TODO in moveItems/ExecuteStep.ts for partial failure handling - Add TODO in deleteFolder/ExecuteStep.ts for rollback consideration
- Use isNameDuplicateInParent instead of getAll for validation - Get connection's parentId to scope the duplicate check - Allow same-name connections in different folders - Add early return for unchanged names
- Add localized ariaLabel to monacoOptions in DataViewPanelJSON - Helps screen readers announce the editor's purpose
- Add localized aria-label to PREVIEW badge in Query Insights tab - Provides screen reader context: 'Query Insights feature is in preview'
- Replace console.error in newConnection/ExecuteStep.ts - Replace console.error in newLocalConnection/ExecuteStep.ts - Replace console.error in renameConnection/ExecuteStep.ts - Use generic error messages to avoid exposing storageId - Errors now properly visible in VS Code Output channel
- Mask password in telemetry immediately after retrieval - Prevents credential exposure if error thrown later
- Wrap subscription in try-catch for proper cleanup on errors - Dispose subscription in catch block before re-throwing - Prevents memory leak if task setup fails
- Add credential and collection existence validation in task init phase - Clear stale clipboard reference when source is no longer valid - Show meaningful error messages to users - Add telemetry for tracking validation failures Moved validation to CopyPasteCollectionTask.init() for cleaner code: - Verifies source cluster credentials are still valid - Verifies source collection still exists - Shows descriptive status during validation
There was a problem hiding this comment.
Pull request overview
This PR refines several 0.7.0-era connection and collection management behaviors, focusing on clearer user feedback, safer validation, and small accessibility/security improvements.
Changes:
- Improved error reporting by routing more failures to the extension output channel and adding new localized strings.
- Strengthened rename-connection duplicate validation (scoped to the same parent folder) and improved copy/paste robustness by validating source connectivity/existence.
- Added accessibility labels in the collection webview and ensured copied connection-string passwords are masked in logs.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/webviews/documentdb/collectionView/components/resultsTab/DataViewPanelJSON.tsx | Adds an ARIA label for the JSON Monaco editor results view. |
| src/webviews/documentdb/collectionView/CollectionView.tsx | Adds an ARIA label to the “PREVIEW” Query Insights badge. |
| src/services/taskService/tasks/copy-and-paste/CopyPasteCollectionTask.ts | Validates source cluster credentials and source collection existence before copy/paste proceeds. |
| src/commands/updateCredentials/ExecuteStep.ts | Switches storage/save failure logging from console.error to output channel with localized messages. |
| src/commands/updateConnectionString/ExecuteStep.ts | Switches update failure logging from console.error to output channel with localized messages. |
| src/commands/pasteCollection/ExecuteStep.ts | Refactors task state subscription disposal logic around task start. |
| src/commands/copyConnectionString/copyConnectionString.ts | Pushes the password into context.valuesToMask before logging/copying. |
| src/commands/connections-view/renameConnection/PromptNewConnectionNameStep.ts | Updates duplicate-name validation to be parent-folder scoped and skips validation when unchanged. |
| src/commands/connections-view/renameConnection/ExecuteStep.ts | Logs rename failures to the output channel with localized messages. |
| src/commands/connections-view/moveItems/ExecuteStep.ts | Adds TODO notes about partial-failure handling/rollback. |
| src/commands/connections-view/deleteFolder/ExecuteStep.ts | Adds TODO notes about partial-failure handling/rollback. |
| l10n/bundle.l10n.json | Adds/removes localized strings to support the updated user-facing/error messages. |
| } catch (error) { | ||
| // Re-throw our own validation errors | ||
| if (error instanceof Error && error.message.includes(vscode.l10n.t('no longer'))) { | ||
| throw error; | ||
| } | ||
| // Wrap other errors (e.g., network issues) | ||
| throw new Error( | ||
| vscode.l10n.t('Failed to validate source collection: {0}', error instanceof Error ? error.message : String(error)), | ||
| ); |
There was a problem hiding this comment.
The catch block uses error.message.includes(vscode.l10n.t('no longer')) to detect “our own” validation errors. This is brittle because it depends on localized text (and translators may not include the exact substring), which can cause user-friendly validation errors to be incorrectly wrapped. Prefer a non-localized signal (e.g., a dedicated error class/type or an error name/code property) and check that instead.
There was a problem hiding this comment.
good catch. that's a bad solution here. to be fixed.
| "No index changes needed at this time.": "No index changes needed at this time.", | ||
| "No index selected.": "No index selected.", | ||
| "No items selected to move.": "No items selected to move.", | ||
| "no longer": "no longer", |
There was a problem hiding this comment.
The "no longer" entry appears to exist only as an internal sentinel for substring matching in error handling logic, not as a user-facing string. This creates unnecessary translation work and makes the sentinel unstable across locales. Removing this key is recommended once the code switches to a non-localized error identifier (e.g., an error class/code).
| if (name.length === 0) { | ||
| return l10n.t('A connection name is required.'); | ||
| } |
There was a problem hiding this comment.
validateNameAvailable only checks name.length === 0, so an input like ' ' will pass validation and later be trimmed to an empty string (context.newConnectionName = newConnectionName.trim()), potentially renaming the connection to an empty name. Use name.trim().length === 0 (and ideally validate against the trimmed value consistently throughout this method).
| // Start the copy-paste task | ||
| void task.start(); | ||
| } catch (error) { | ||
| // Ensure subscription is disposed if task start fails | ||
| subscription?.dispose(); | ||
| throw error; | ||
| } |
There was a problem hiding this comment.
task.start() returns a Promise and can reject if onInitialize() throws (e.g., the new source validation errors). Because it’s invoked with void task.start() inside a try/catch, failures won’t be caught here and can become unhandled promise rejections, leaving the onDidChangeState subscription undisposed. Handle the Promise explicitly (e.g., await task.start() since it resolves once started, or attach a .catch() that disposes the subscription and reports the error).
There was a problem hiding this comment.
good catch. an obvious oversight.
1 participant
This pull request introduces several improvements and fixes related to error handling, user feedback, and validation for connection and collection management features. The main changes include more precise error messages, enhanced validation for renaming connections, improved resource cleanup in asynchronous operations, and developer notes for future error handling improvements.
Error handling and user feedback improvements:
Validation and duplicate checking:
Resource management and async operation safety:
Developer guidance and future improvements:
Other minor improvements:
These changes collectively improve the reliability, usability, and maintainability of connection management features in the extension.