Feature: Easy Data Migrations (Collection Copy-and-Paste) #170
Conversation
Co-authored-by: tnaum-ms <[email protected]>
Co-authored-by: tnaum-ms <[email protected]>
Co-authored-by: tnaum-ms <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
…aste-2-implement-basic-copy-and-paste-task
Co-authored-by: Copilot <[email protected]>
…-documentdb into dev/xingfan/111-copy-and-paste-2-implement-basic-copy-and-paste-task
…into dev/xingfan/111-copy-and-paste-2-implement-basic-copy-and-paste-task
|
@copilot This branch is out-of-date with the base branch. Merge the latest changes from next into this branch. Resolve conflicts: |
tnaum-ms
force-pushed
the
feature/copy-and-paste
branch
from
d1706ce to
b9346e3
Compare
# Conflicts: # .github/copilot-instructions.md # l10n/bundle.l10n.json # src/commands/addConnectionFromRegistry/addConnectionFromRegistry.ts # src/commands/newConnection/ExecuteStep.ts # src/commands/removeConnection/removeConnection.ts # src/documentdb/ClustersExtension.ts
…/vscode-documentdb into feature/copy-and-paste
…n" items based on existing connections
…tion steps - Added ConfirmDeleteStep to prompt user for confirmation before deletion. - Introduced VerifyStep to check for running tasks using connections in the folder. - Created ExecuteStep to handle the actual deletion of folders and connections. - Defined DeleteFolderWizardContext to manage state throughout the wizard. - Refactored deleteFolder command to utilize the new wizard structure. - Updated tests to cover new functionality and ensure correct behavior during deletion.
| } | ||
|
|
||
| // Use Intl.NumberFormat for compact notation | ||
| const formatter = new Intl.NumberFormat('en', { |
There was a problem hiding this comment.
The locale is hardcoded to 'en'. Consider using the user's VS Code locale for better internationalization. You can access this via vscode.env.language or use 'en-US' as a more specific locale identifier.
| public loadDocumentCount(): void { | ||
| // Skip if already loading or already loaded | ||
| if (this.isLoadingCount || this.documentCount !== undefined) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Race condition: this.isLoadingCount is set to true after the check, creating a small window where multiple calls could pass the check. Set this.isLoadingCount = true immediately after entering the function body and before the check to prevent concurrent loads.
| await callWithTelemetryAndErrorHandling('taskService.taskExecution', async (context: IActionContext) => { | ||
| // Add base task properties with task_ prefix | ||
| context.telemetry.properties.task_id = this.id; | ||
| context.telemetry.properties.task_type = this.constructor.name; |
There was a problem hiding this comment.
Inconsistent telemetry property: Line 279 uses this.type but line 321 uses this.constructor.name. These should both use this.type for consistency across initialization and execution telemetry.
| } | ||
|
|
||
| // Move to next batch - account for both partial progress and final result | ||
| const totalProcessedInBatch = partialProgressCount + result.processedCount; |
There was a problem hiding this comment.
The variable name totalProcessedInBatch is misleading. This represents the number of documents to skip from pendingDocs, not a total count. Consider renaming to documentsToSkip or processedSoFar for clarity.
| let cleanup: () => void; | ||
|
|
||
| if (abortSignal) { |
There was a problem hiding this comment.
Variable cleanup is used before assignment. If abortSignal is falsy, line 265's cleanup() will execute before cleanup is assigned on line 284. Move the cleanup function declaration before the timeout setup or initialize it with a no-op function.
| * @param _actionContext Optional action context for telemetry (currently unused) | ||
| * @returns Promise resolving to the estimated document count | ||
| */ | ||
| protected async countDocumentsInDatabase(_signal?: AbortSignal, _actionContext?: IActionContext): Promise<number> { |
There was a problem hiding this comment.
The _signal parameter is unused despite being part of the interface contract. While this may be intentional for the current implementation, consider adding a comment explaining why AbortSignal isn't used with estimateDocumentCount, or add support for cancellation if the underlying API supports it.
| } | ||
| return `${existingLabel} (1)`; | ||
| // Fallback to prevent endless loop if regex fails - use timestamp for guaranteed uniqueness | ||
| return `${existingLabel} (${Date.now()})`; |
There was a problem hiding this comment.
Using Date.now() creates labels like 'My Collection (1737028800000)' which is not user-friendly. Consider using a more readable timestamp format or a simpler counter. The timestamp is also guaranteed unique but unnecessarily long.
Easy Data Migrations for DocumentDB and MongoDB databases is a feature designed to simplify small-scale data migrations by leveraging a user-friendly copy-and-paste experience. This feature is ideal for smaller datasets where all data can be moved through the user's local machine.
For details of this PR review: #63
closes #63