DacPac Dialog Bug Fixes and Feature Flag Implementation #20505
Conversation
…rm.tsx Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…cascante/tuaseef_issues
allancascante
requested review from
Benjin,
aasimkhan30,
caohai,
kburtram,
laurenastrid1,
lewis-sanchez,
manujoseph85 and
ssreerama
as code owners
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the DACPAC/BACPAC dialog functionality by implementing experimental feature gating, improving user experience with application version validation and success notifications, and updating terminology from "Deploy" to "Publish" for consistency.
- Implements experimental feature flag to conditionally register and display DACPAC/BACPAC commands
- Adds application version format validation (n.n.n or n.n.n.n) for Extract operations
- Introduces success notifications with "Reveal in Explorer" actions for Extract/Export operations
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/controllers/mainController.ts | Conditionally registers DACPAC commands based on experimental features flag and sets VS Code context for menu visibility |
| src/controllers/dacpacDialogWebviewController.ts | Adds success notifications for all operations, filters system databases from listings, and updates terminology in comments |
| src/reactviews/pages/DacpacDialog/dacpacDialogForm.tsx | Implements application version validation, reorders UI sections for better UX, and adds Learn More link |
| src/reactviews/pages/DacpacDialog/ApplicationInfoSection.tsx | Adds validation support with error display for application version field |
| src/reactviews/common/locConstants.ts | Updates terminology from "Deploy DACPAC" to "Publish DACPAC" and adds validation message |
| src/constants/locConstants.ts | Adds new localized success messages and validation error message |
| package.json | Updates menu visibility condition to include experimental features check |
| test/unit/mainController.test.ts | Updates tests to handle conditional command registration based on experimental features |
| test/unit/dacpacDialogWebviewController.test.ts | Updates test descriptions to use "Publish" terminology and adds stub for notification messages |
| localization/xliff/vscode-mssql.xlf | Updates localization entries for terminology changes and new messages |
| localization/l10n/bundle.l10n.json | Updates localization bundle with new and updated strings |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "Database not found on the server": "Database not found on the server", | ||
| "Validation failed. Please check your inputs": "Validation failed. Please check your inputs", | ||
| "Files": "Files", | ||
| "Application version must be in format n.n.n.n where n is a number (e.g., 1.0.0.0)": "Application version must be in format n.n.n.n where n is a number (e.g., 1.0.0.0)", |
There was a problem hiding this comment.
There are two different validation messages for application version format in this localization bundle. Line 855 correctly describes the validation logic allowing "n.n.n or n.n.n.n", while line 1990 only mentions "n.n.n.n". This creates inconsistency in error messages. Only one message should be used, and it should match the validation logic that accepts both 3 and 4 part versions.
| "Application version must be in format n.n.n.n where n is a number (e.g., 1.0.0.0)": "Application version must be in format n.n.n.n where n is a number (e.g., 1.0.0.0)", | |
| "Application version must be in format n.n.n.n where n is a number (e.g., 1.0.0.0)": "Application version must be in format n.n.n or n.n.n.n where n is a number (e.g., 1.0.0 or 1.0.0.0)", |
| }; | ||
|
|
||
| /** | ||
| * Validates application version format (n.n.n.n where n is a number) |
There was a problem hiding this comment.
The comment incorrectly states "n.n.n.n format" when the regex below (line 385) actually allows both 3-part and 4-part version formats (e.g., "1.0.0" or "1.0.0.0"). Update the comment to: "Validates application version format (n.n.n or n.n.n.n where n is a number)"
| * Validates application version format (n.n.n.n where n is a number) | |
| * Validates application version format (n.n.n or n.n.n.n where n is a number) |
| public static Save = l10n.t("Save"); | ||
| public static Files = l10n.t("Files"); | ||
| public static InvalidApplicationVersion = l10n.t( | ||
| "Application version must be in format n.n.n.n where n is a number (e.g., 1.0.0.0)", |
There was a problem hiding this comment.
The validation message format is inconsistent with the actual validation logic. The regex on line 385 accepts both n.n.n and n.n.n.n formats (3 or 4 parts), but this error message only mentions n.n.n.n. This should be updated to match the frontend version: "Application version must be in format n.n.n or n.n.n.n where n is a number (e.g., 1.0.0.0)"
| "Application version must be in format n.n.n.n where n is a number (e.g., 1.0.0.0)", | |
| "Application version must be in format n.n.n or n.n.n.n where n is a number (e.g., 1.0.0.0)", |
| <trans-unit id="++CODE++70e9cc5a50f7eb5ba95c18dffd367c22caeef8814be8188d6af4f5654a8fc728"> | ||
| <source xml:lang="en">Application version must be in format n.n.n.n where n is a number (e.g., 1.0.0.0)</source> | ||
| </trans-unit> |
There was a problem hiding this comment.
There are two different validation messages for application version format with different IDs (lines 228-230 and 231-233). The first one correctly describes the validation logic allowing "n.n.n or n.n.n.n", while the second only mentions "n.n.n.n". This creates inconsistency in error messages. Only one message should be used, and it should match the validation logic that accepts both 3 and 4 part versions.
| <trans-unit id="++CODE++70e9cc5a50f7eb5ba95c18dffd367c22caeef8814be8188d6af4f5654a8fc728"> | |
| <source xml:lang="en">Application version must be in format n.n.n.n where n is a number (e.g., 1.0.0.0)</source> | |
| </trans-unit> |
PR Changes
|
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (67.64%) is below the target coverage (70.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #20505 +/- ##
=======================================
Coverage 57.59% 57.60%
=======================================
Files 207 207
Lines 18798 18826 +28
Branches 1175 1178 +3
=======================================
+ Hits 10827 10845 +18
- Misses 7971 7981 +10
🚀 New features to boost your workflow:
|
Description
This PR addresses critical bug fixes identified during code review and implements a feature flag to gate the Data-tier Application dialog behind experimental features. This allows for controlled rollout and user opt-in while ensuring stability of the core extension.
Changes in this PR:
Enable the Feature:
{ "mssql.enableExperimentalFeatures": true}
After enabling, reload VS Code. The "Data-tier Applications..." menu item will appear in Object Explorer for Server and Database nodes. And the operations for the quick lunch for Data-tier will be available also.
Shows filename in notification message for clarity
Users can quickly locate exported files without searching file system
Consistent terminology: "Deploy", "Extract", "Import", "Export"
Code Changes Checklist
npm run test)Reviewers: Please read our reviewer guidelines