Conversation
laurenastrid1
requested review from
Benjin,
aasimkhan30,
allancascante,
caohai,
kburtram,
lewis-sanchez,
manujoseph85 and
ssreerama
as code owners
There was a problem hiding this comment.
Pull request overview
This pull request adds a flat file import feature to the VS Code MSSQL extension, porting functionality from Azure Data Studio. The feature allows users to import CSV/TXT files into SQL Server databases through a multi-step wizard interface.
Changes:
- Adds new SqlOpsClient service infrastructure with service downloader support for the flat file import backend service
- Implements a React-based multi-step wizard UI for flat file import (file selection, preview, column configuration, and import summary)
- Integrates flat file import command into Object Explorer context menus for servers and databases
- Includes comprehensive unit tests for all new components
Reviewed changes
Copilot reviewed 32 out of 33 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| extensions/mssql/src/sqlOps/* | Core service client infrastructure for SQL Operations services, including client interfaces, service API manager, and flat file feature registration |
| extensions/mssql/src/controllers/flatFileImportController.ts | Main controller managing flat file import workflow and state |
| extensions/mssql/src/reactviews/pages/FlatFileImport/* | React UI components for the multi-step import wizard |
| extensions/mssql/src/models/contracts/flatFile.ts | TypeScript contract definitions for flat file service requests/responses |
| extensions/mssql/src/controllers/mainController.ts | Integration of flat file service initialization and command registration |
| extensions/mssql/package.json | Command and menu contributions for flat file import feature |
| extensions/mssql/flatFileConfig.json | Configuration for downloading the flat file import service binaries |
| extensions/mssql/yarn.lock | New dependency @microsoft/ads-service-downloader and its transitive dependencies |
| localization/xliff/vscode-mssql.xlf | Localization strings for the flat file import feature |
| extensions/mssql/test/unit/* | Comprehensive unit tests for new components |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import { TelemetryActions, TelemetryViews } from "../sharedInterfaces/telemetry"; | ||
|
|
||
| /** | ||
| * Controller for the Add Firewall Rule dialog |
There was a problem hiding this comment.
The JSDoc comment describes this class as "Controller for the Add Firewall Rule dialog" but this is actually the FlatFileImportController. The comment should be updated to accurately describe this controller's purpose.
| * Controller for the Add Firewall Rule dialog | |
| * Controller for the Flat File Import dialog |
| private statusView: vscode.StatusBarItem; | ||
|
|
||
| constructor(private outputChannel: vscode.OutputChannel) { | ||
| this.statusView = vscode.window.createStatusBarItem(vscode.StatusBarAlignment.Left); |
There was a problem hiding this comment.
The statusView StatusBarItem is created in the constructor but is never disposed. This should be disposed to prevent resource leaks. Consider adding a dispose method to SqlOpsClient that disposes of the statusView, or dispose it when it's no longer needed.
| const flatFileImportDialog = new FlatFileImportController( | ||
| this._context, | ||
| this._vscodeWrapper, | ||
| SqlToolsServerClient.instance, | ||
| this.connectionManager, | ||
| this.flatFileProvider, |
There was a problem hiding this comment.
The command handler does not check if this.flatFileProvider is defined before passing it to FlatFileImportController. If the flat file service fails to start during initialization (line 859 in mainController.ts), flatFileProvider could be undefined, leading to runtime errors when users try to use the import feature. Consider adding a check to ensure flatFileProvider is available before creating the controller, and showing an appropriate error message if it's not.
PR Changes
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #21104 +/- ##
==========================================
+ Coverage 67.89% 68.31% +0.42%
==========================================
Files 230 237 +7
Lines 22394 22856 +462
Branches 2971 3009 +38
==========================================
+ Hits 15204 15614 +410
- Misses 7059 7106 +47
- Partials 131 136 +5
🚀 New features to boost your workflow:
|
| "mssql.showChangelogOnUpdate": "Show changelog after extension updates", | ||
| "mssql.backupDatabase": "Backup Database (Preview)" | ||
| "mssql.backupDatabase": "Backup Database (Preview)", | ||
| "mssql.flatFileImport": "Import (Preview)" |
There was a problem hiding this comment.
Import flat file?
Import can be a little confusing as it coincides with import export wizard.
https://learn.microsoft.com/en-us/sql/integration-services/import-export-data/start-the-sql-server-import-and-export-wizard?view=sql-server-ver17
| @@ -0,0 +1,16 @@ | |||
| { | |||
| "downloadUrl": "https://sqlopsextensions.blob.core.windows.net/extensions/import/service/{#version#}/flatfileimportservice-{#fileName#}", | |||
There was a problem hiding this comment.
Just convert it to .ts file and put this code. Let's not pollute the root. Look at config.ts file for downloading sts.
There was a problem hiding this comment.
can we extract common code out of service client and this for downloading and status? We should have one plug and play component. As this just dups a lot of functionality.
| node.connectionProfile, | ||
| ); | ||
|
|
||
| const databases = await this.connectionManager.listDatabases(connectionUri); |
There was a problem hiding this comment.
By preloading the databases asynchronously, there's a risk that this call takes a while and then the dialog takes a long time to open. Probably better to pass in the connection Uri to the FFI controller, and have that display a (hopefully brief) loading state as it tries to fetch the databases.
| /*--------------------------------------------------------------------------------------------- | ||
| * Copyright (c) Microsoft Corporation. All rights reserved. | ||
| * Licensed under the MIT License. See License.txt in the project root for license information. | ||
| *--------------------------------------------------------------------------------------------*/ |
There was a problem hiding this comment.
flatFileImportWebviewController.ts for consistency
| FlatFileImportFormItemSpec, | ||
| FlatFileImportReducers | ||
| > { | ||
| public readonly IMPORT_FILE_TYPES = ["csv", "txt"]; |
There was a problem hiding this comment.
Can be a const array outside of the class so it's only defined once.
| > { | ||
| public readonly IMPORT_FILE_TYPES = ["csv", "txt"]; | ||
| // Default form action ; this is used as part of the form reducer to handle default logic | ||
| private baseFormActionReducer = this["_reducerHandlers"].get("formAction"); |
There was a problem hiding this comment.
Maybe clarify in the ocmment that this is being saved because you're overriding the formAction reducer below?
| public readonly IMPORT_FILE_TYPES = ["csv", "txt"]; | ||
| // Default form action ; this is used as part of the form reducer to handle default logic | ||
| private baseFormActionReducer = this["_reducerHandlers"].get("formAction"); | ||
| constructor( |
There was a problem hiding this comment.
Nit: leave a blank line before block definitions (functions, constructors) for readability.
| expect(opts.args).to.include("--log-dir"); | ||
| expect(opts.args).to.include("/mock/log"); | ||
| expect(opts.transport).to.exist; | ||
| expect(spyConfig.calledWith("mssql")).to.be.true; |
| // LOG_EMITTED (simulate info, should not append) | ||
| (mockOutputChannel.appendLine as sinon.SinonStub).resetHistory(); | ||
| handler("log_emitted", 1, "This is info message"); | ||
| expect((mockOutputChannel.appendLine as sinon.SinonStub).called).to.be.false; |
| @@ -0,0 +1,16 @@ | |||
| { | |||
| "downloadUrl": "https://sqlopsextensions.blob.core.windows.net/extensions/import/service/{#version#}/flatfileimportservice-{#fileName#}", | |||
There was a problem hiding this comment.
Uh oh. We're gonna have to host this somewhere else (STS github), 'cause this storage account is getting shut down with the ADS retirement. Please do not check this PR in without this URL getting updated to a different download location, 'cause I feel like this is going to be an easy one ot miss later.
| "schema": { "type": "string" }, | ||
| "name": { "type": "string" }, | ||
| "id": { "type": "string" } | ||
| "schema": { |
There was a problem hiding this comment.
Did you change anything in this chunk of reformats? If not, does the PR build pass with these unrelated changes reverted, or does the linter require these in this PR?
| "@azure/ms-rest-azure-env": "^2.0.0", | ||
| "@azure/msal-common": "^14.14.0", | ||
| "@azure/msal-node": "^2.12.0", | ||
| "@microsoft/ads-service-downloader": "^1.2.1", |
There was a problem hiding this comment.
Can you double-check to see how we did the download for STS, and if that's at all reusable?
There was a problem hiding this comment.
Please reduce the padding for table rows. There is too much top and bottom padding per row.
aasimkhan30
left a comment
aasimkhan30
left a comment
There was a problem hiding this comment.
Looks mostly good. Feedback regarding cleanups can be a follow up PR. Please look at copilot review to fix some name related typos.
4 participants
Pull Request Template – vscode-mssql
Description
Added the flat file dialog from ADS
Code Changes Checklist
npm run test)Reviewers: Please read our reviewer guidelines