Skip to content

Conversation

@allancascante
Copy link
Contributor

@allancascante allancascante commented Nov 20, 2025

Description

This PR refactors the task completion notification system to eliminate duplicate notifications for DacFx operations (Export BACPAC, Extract DACPAC, Import BACPAC, Deploy DACPAC) and provides a generic, configurable solution for custom task completion handlers, as reported in this bug.

Problem
Previously, DacFx operations showed two notifications:

A custom notification with action button from dacpacDialogWebviewController
A generic completion notification from sqlTasksService

Solution

Centralized all task completion notification logic in sqlTasksService with a configurable handler registration system.

Code Changes Checklist

  • New or updated unit tests added
  • All existing tests pass (npm run test)
  • Code follows contributing guidelines
  • Telemetry/logging updated if relevant
  • No regressions or UX breakage

Reviewers: Please read our reviewer guidelines

@github-actions
Copy link

github-actions bot commented Nov 20, 2025

PR Changes

Category Target Branch PR Branch Difference
vscode-mssql VSIX 5262 KB 5262 KB ⚪ 0 KB ( 0% )
sql-database-projects VSIX 5627 KB 5627 KB ⚪ 0 KB ( 0% )

@codecov-commenter
Copy link

codecov-commenter commented Nov 20, 2025

Codecov Report

❌ Patch coverage is 98.66667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 62.94%. Comparing base (3109cca) to head (fc1a170).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
extensions/mssql/src/controllers/mainController.ts 0.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #20651      +/-   ##
==========================================
+ Coverage   62.65%   62.94%   +0.29%     
==========================================
  Files         209      209              
  Lines       19350    19390      +40     
  Branches     2461     2464       +3     
==========================================
+ Hits        12124    12206      +82     
+ Misses       7146     7102      -44     
- Partials       80       82       +2     
Files with missing lines Coverage Δ
extensions/mssql/src/constants/constants.ts 100.00% <100.00%> (ø)
...l/src/controllers/dacpacDialogWebviewController.ts 92.16% <ø> (+0.64%) ⬆️
extensions/mssql/src/services/dacFxService.ts 53.94% <100.00%> (+48.68%) ⬆️
extensions/mssql/src/services/sqlTasksService.ts 82.35% <100.00%> (+45.88%) ⬆️
extensions/mssql/src/controllers/mainController.ts 16.88% <0.00%> (ø)

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

/**
* Helper method to register task completion handlers for DacPac operations
*/
private registerTaskCompletionHandlers(): void {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this method is only used for DACPAC-related completion handlers, could we rename it to something more specific like registerDacpacTaskCompletionHandlers?

Also, I’m wondering if we should start moving some of this logic out of mainController into feature-specific modules (e.g. a DACPAC-specific file) and just import/register them here. This file is starting to get quite large, and splitting by feature area might make it easier to maintain.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking this registration should be done by the dacpacDialogWebviewController when it launches the first time. It wouldn't need to unregister upon webview disposal because that would revert to the less helpful task completion.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or maybe the dacfxService constructor would be even better.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Data-tier Application dialog has duplicate notifications

5 participants