Extension UI cleanup with multiple SQL extension#21101
Extension UI cleanup with multiple SQL extension#21101aasimkhan30 wants to merge 30 commits intomainfrom
Conversation
…and UI visibility
There was a problem hiding this comment.
Pull request overview
This PR implements a URI ownership coordination system between the MSSQL and PostgreSQL VS Code extensions to prevent duplicate UI elements (status bar items, CodeLens, editor buttons) when both extensions are installed. The coordination is achieved through a shared API that allows extensions to query which extension owns a given file URI.
Changes:
- Introduced
UriOwnershipCoordinatorclass to manage coordination with other database extensions - Added command name suffixes "(MSSQL)" to disambiguate from PostgreSQL commands in the command palette
- Implemented UI hiding logic in status bar, CodeLens, and editor menus based on URI ownership
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| extensions/mssql/src/uriOwnership.ts | New coordinator class that manages URI ownership API and listens for ownership changes from other extensions |
| extensions/mssql/typings/vscode-mssql.d.ts | Added UriOwnershipApi interface to the public API |
| extensions/mssql/src/extension.ts | Instantiates and initializes the URI ownership coordinator |
| extensions/mssql/src/views/statusView.ts | Hides status bar items when active file is owned by another extension |
| extensions/mssql/src/queryResult/sqlCodeLensProvider.ts | Hides CodeLens when URI is owned by another extension |
| extensions/mssql/package.json | Added context-based visibility conditions to editor menu commands |
| extensions/mssql/src/controllers/mainController.ts | Added ownership checks to prevent command execution on files owned by other extensions |
| extensions/mssql/src/constants/locConstants.ts | Added localized error message for files owned by other extensions |
| extensions/mssql/package.nls.json | Updated command titles with "(MSSQL)" suffix |
| localization/xliff/vscode-mssql.xlf | Updated localization resources |
| extensions/mssql/l10n/bundle.l10n.json | Updated l10n bundle with new messages |
Comments suppressed due to low confidence (2)
extensions/mssql/package.json:440
- For defensive programming and consistency, consider adding the
!mssql.hideUIElementscondition to other MSSQL-specific commands in the editor/title menu (disconnect, changeConnection, changeDatabase, showEstimatedPlan, etc.), not just runQuery and connect. While theresource in mssql.connectionscheck should prevent these buttons from showing when a file is owned by another extension, adding the explicit hideUIElements check would provide an extra layer of protection against potential race conditions during connection handoff between extensions.
"editor/title": [
{
"command": "mssql.runQuery",
"when": "editorLangId == sql && !isInDiffEditor && resourcePath not in mssql.runningQueries && !mssql.hideUIElements",
"group": "navigation@1"
},
{
"command": "mssql.cancelQuery",
"when": "editorLangId == sql && !isInDiffEditor && resourcePath in mssql.runningQueries",
"group": "navigation@2"
},
{
"command": "mssql.revealQueryResult",
"when": "editorLangId == sql && resource in mssql.connections && !isInDiffEditor",
"group": "navigation@2"
},
{
"command": "mssql.connect",
"when": "editorLangId == sql && !isInDiffEditor && resource not in mssql.connections && !mssql.hideUIElements",
"group": "navigation@3"
},
{
"command": "mssql.disconnect",
"when": "editorLangId == sql && !isInDiffEditor && resource in mssql.connections && resourcePath not in mssql.runningQueries",
"group": "navigation@3"
},
{
"command": "mssql.changeConnection",
"when": "editorLangId == sql && !isInDiffEditor && resource in mssql.connections && resourcePath not in mssql.runningQueries",
"group": "navigation@4"
},
{
"command": "mssql.changeDatabase",
"when": "editorLangId == sql && !isInDiffEditor && resource in mssql.connections && resourcePath not in mssql.runningQueries",
"group": "navigation@5"
},
{
"command": "mssql.showEstimatedPlan",
"when": "editorLangId == sql && !isInDiffEditor && resource in mssql.connections && resourcePath not in mssql.runningQueries",
"group": "navigation@10"
},
{
"command": "mssql.enableActualPlan",
"when": "editorLangId == sql && !isInDiffEditor && resource in mssql.connections && (resource not in mssql.executionPlan.urisWithActualPlanEnabled) && (resourcePath not in mssql.runningQueries)",
"group": "navigation@11"
},
{
"command": "mssql.disableActualPlan",
"when": "editorLangId == sql && !isInDiffEditor && resource in mssql.connections && (resource in mssql.executionPlan.urisWithActualPlanEnabled) && (resourcePath not in mssql.runningQueries)",
"group": "navigation@12"
}
extensions/mssql/src/controllers/mainController.ts:241
- The cmdChangeConnection command handler doesn't check for URI ownership like cmdConnect and cmdRunQuery do. While the editor menu buttons are hidden via package.json conditions, users can still invoke this command from the command palette. If a file is owned by another extension (e.g., PostgreSQL), users could potentially invoke "Change Connection (MSSQL)" from the command palette and get unexpected behavior. Consider adding the same ownership check here for consistency.
this.registerCommand(Constants.cmdChangeConnection);
this._event.on(Constants.cmdChangeConnection, () => {
void this.runAndLogErrors(this.onNewConnection());
});
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
PR Changes
|
Codecov Report❌ Patch coverage is ❌ Your patch status has failed because the patch coverage (32.60%) 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 #21101 +/- ##
==========================================
- Coverage 72.73% 72.56% -0.18%
==========================================
Files 321 323 +2
Lines 95281 95690 +409
Branches 5236 5249 +13
==========================================
+ Hits 69304 69434 +130
- Misses 25977 26256 +279
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 12 changed files in this pull request and generated 8 comments.
Comments suppressed due to low confidence (1)
extensions/mssql/package.json:440
- Several editor/title menu items that should be hidden when the URI is owned by another extension are missing the
&& !mssql.hideUIElementscondition:
- mssql.disconnect (line 413)
- mssql.changeConnection (line 418)
- mssql.changeDatabase (line 423)
- mssql.revealQueryResult (line 403)
- mssql.cancelQuery (line 398)
- mssql.showEstimatedPlan (line 428)
- mssql.enableActualPlan (line 433)
- mssql.disableActualPlan (line 438)
These commands all operate on the active editor and should not be visible when the file is owned by a coordinating extension like PostgreSQL, for consistency with the mssql.runQuery and mssql.connect commands that already have this condition.
{
"command": "mssql.cancelQuery",
"when": "editorLangId == sql && !isInDiffEditor && resourcePath in mssql.runningQueries",
"group": "navigation@2"
},
{
"command": "mssql.revealQueryResult",
"when": "editorLangId == sql && resource in mssql.connections && !isInDiffEditor",
"group": "navigation@2"
},
{
"command": "mssql.connect",
"when": "editorLangId == sql && !isInDiffEditor && resource not in mssql.connections && !mssql.hideUIElements",
"group": "navigation@3"
},
{
"command": "mssql.disconnect",
"when": "editorLangId == sql && !isInDiffEditor && resource in mssql.connections && resourcePath not in mssql.runningQueries",
"group": "navigation@3"
},
{
"command": "mssql.changeConnection",
"when": "editorLangId == sql && !isInDiffEditor && resource in mssql.connections && resourcePath not in mssql.runningQueries",
"group": "navigation@4"
},
{
"command": "mssql.changeDatabase",
"when": "editorLangId == sql && !isInDiffEditor && resource in mssql.connections && resourcePath not in mssql.runningQueries",
"group": "navigation@5"
},
{
"command": "mssql.showEstimatedPlan",
"when": "editorLangId == sql && !isInDiffEditor && resource in mssql.connections && resourcePath not in mssql.runningQueries",
"group": "navigation@10"
},
{
"command": "mssql.enableActualPlan",
"when": "editorLangId == sql && !isInDiffEditor && resource in mssql.connections && (resource not in mssql.executionPlan.urisWithActualPlanEnabled) && (resourcePath not in mssql.runningQueries)",
"group": "navigation@11"
},
{
"command": "mssql.disableActualPlan",
"when": "editorLangId == sql && !isInDiffEditor && resource in mssql.connections && (resource in mssql.executionPlan.urisWithActualPlanEnabled) && (resourcePath not in mssql.runningQueries)",
"group": "navigation@12"
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 13 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 27 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| private _refreshCoordinatingExtensions(): void { | ||
| const newExtensions = discoverCoordinatingExtensions(this._context.extension.id); | ||
|
|
||
| // Find newly added extensions | ||
| for (const extInfo of newExtensions) { | ||
| if (!this._coordinatingExtensionApis.has(extInfo.extensionId)) { | ||
| const extension = vscode.extensions.getExtension(extInfo.extensionId); | ||
| if (extension?.isActive) { | ||
| this._registerCoordinatingExtensionApi(extInfo.extensionId, extension.exports); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| this._coordinatingExtensions = newExtensions; | ||
| } |
There was a problem hiding this comment.
The _refreshCoordinatingExtensions method does not handle the case where an extension is uninstalled or deactivated. When an extension is removed, it remains in _coordinatingExtensionApis Map and its event listener remains registered in context.subscriptions, potentially causing errors when the extension tries to call the API or when events fire. The method should check for removed extensions and clean up their entries from _coordinatingExtensionApis.
… and adding releaseUri callback
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 28 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…oord # Conflicts: # extensions/mssql/package.json # extensions/mssql/src/controllers/mainController.ts # extensions/mssql/src/extension.ts # extensions/mssql/src/queryResult/sqlCodeLensProvider.ts
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 25 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <source xml:lang="en">Cancel Query (MSSQL)</source> | ||
| </trans-unit> | ||
| <trans-unit id="mssql.changeConnection"> | ||
| <source xml:lang="en">Change Connection</source> | ||
| <source xml:lang="en">Change Connection (MSSQL)</source> | ||
| </trans-unit> | ||
| <trans-unit id="mssql.changeDatabase"> | ||
| <source xml:lang="en">Change Database</source> | ||
| <source xml:lang="en">Change Database (MSSQL)</source> |
There was a problem hiding this comment.
The localized translation files (de, es, fr, it, ja, ko, pt-BR, ru, zh-Hans, zh-Hant) have not been updated to reflect the command name changes. The source file vscode-mssql.xlf shows updated strings like "Execute Query (MSSQL)" but the German translation file vscode-mssql.de.xlf still has the old English source text "Cancel Query" without "(MSSQL)" suffix at line 8733.
According to the localization workflow memory, when adding or updating trans-units in vscode-mssql.xlf, they must be added/updated in all 10 localized files with target state="new" containing the English source text. This ensures translators know these strings need translation.
| "command": "mssql.cancelQuery", | ||
| "when": "editorLangId == sql && !isInDiffEditor && resourcePath in mssql.runningQueries", | ||
| "group": "navigation@2" | ||
| }, | ||
| { | ||
| "command": "mssql.revealQueryResult", | ||
| "when": "editorLangId == sql && resource in mssql.connections && !isInDiffEditor", | ||
| "group": "navigation@2" | ||
| }, | ||
| { | ||
| "command": "mssql.connect", | ||
| "when": "editorLangId == sql && !isInDiffEditor && resource not in mssql.connections && !mssql.hideUIElements", | ||
| "group": "navigation@3" | ||
| }, | ||
| { | ||
| "command": "mssql.disconnect", | ||
| "when": "editorLangId == sql && !isInDiffEditor && resource in mssql.connections && resourcePath not in mssql.runningQueries", | ||
| "group": "navigation@3" | ||
| }, | ||
| { | ||
| "command": "mssql.changeConnection", | ||
| "when": "editorLangId == sql && !isInDiffEditor && resource in mssql.connections && resourcePath not in mssql.runningQueries", | ||
| "group": "navigation@4" | ||
| }, | ||
| { | ||
| "command": "mssql.changeDatabase", | ||
| "when": "editorLangId == sql && !isInDiffEditor && resource in mssql.connections && resourcePath not in mssql.runningQueries", | ||
| "group": "navigation@5" | ||
| }, | ||
| { | ||
| "command": "mssql.showEstimatedPlan", | ||
| "when": "editorLangId == sql && !isInDiffEditor && resource in mssql.connections && resourcePath not in mssql.runningQueries", | ||
| "group": "navigation@10" | ||
| }, | ||
| { | ||
| "command": "mssql.enableActualPlan", | ||
| "when": "editorLangId == sql && !isInDiffEditor && resource in mssql.connections && (resource not in mssql.executionPlan.urisWithActualPlanEnabled) && (resourcePath not in mssql.runningQueries)", | ||
| "group": "navigation@11" | ||
| }, | ||
| { | ||
| "command": "mssql.disableActualPlan", | ||
| "when": "editorLangId == sql && !isInDiffEditor && resource in mssql.connections && (resource in mssql.executionPlan.urisWithActualPlanEnabled) && (resourcePath not in mssql.runningQueries)", | ||
| "group": "navigation@12" |
There was a problem hiding this comment.
Several editor/title menu items are missing the !mssql.hideUIElements condition and will remain visible when a file is owned by a coordinating extension (e.g., PostgreSQL). These commands should be hidden to avoid user confusion:
- Line 407:
mssql.disconnect- shown when connected - Line 412:
mssql.changeConnection- shown when connected - Line 417:
mssql.changeDatabase- shown when connected - Line 422:
mssql.showEstimatedPlan- shown when connected - Line 427:
mssql.enableActualPlan- shown when connected - Line 432:
mssql.disableActualPlan- shown when connected - Line 392:
mssql.cancelQuery- shown when query is running - Line 397:
mssql.revealQueryResult- shown when results exist
These should include && !mssql.hideUIElements in their "when" clause for consistency with mssql.runQuery (line 387) and mssql.connect (line 402).
packages/vscode-sql-common/README.md
Outdated
| /** Optional localized default warning message factory */ | ||
| fileOwnedByOtherExtensionMessage?: (owningExtensionDisplayName: string) => string; | ||
|
|
There was a problem hiding this comment.
The README.md documents a fileOwnedByOtherExtensionMessage configuration option at line 145, but this property is not present in the actual UriOwnershipConfig interface defined in types.ts (lines 54-87). This is a documentation error - the option doesn't exist in the implementation.
The warning message is actually generated in isActiveEditorOwnedByOtherExtensionWithWarning() at uriOwnershipCoordinator.ts lines 215-217 using a default format, and can be customized by passing a warningMessage parameter to that method.
| /** Optional localized default warning message factory */ | |
| fileOwnedByOtherExtensionMessage?: (owningExtensionDisplayName: string) => string; | |
…ation details and add usage instructions
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 14 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const newExtensions = discoverCoordinatingExtensions(this._context.extension.id); | ||
|
|
There was a problem hiding this comment.
_refreshCoordinatingExtensions adds newly discovered APIs but never removes entries from _coordinatingExtensionApis for extensions that are no longer installed/disabled. That can leave stale ownership checks. Consider pruning the map to the currently discovered extension IDs during refresh.
| const newExtensions = discoverCoordinatingExtensions(this._context.extension.id); | |
| const newExtensions = discoverCoordinatingExtensions(this._context.extension.id); | |
| const discoveredIds = new Set(newExtensions.map(ext => ext.extensionId)); | |
| // Remove APIs for extensions that are no longer discovered. | |
| for (const existingId of this._coordinatingExtensionApis.keys()) { | |
| if (!discoveredIds.has(existingId)) { | |
| this._coordinatingExtensionApis.delete(existingId); | |
| } | |
| } |
| // TODO: Move URI ownership core/coordinator logic to a shared package. | ||
| const PACKAGE_JSON_COMMON_FEATURES_KEY = "vscode-sql-common-features"; | ||
| const SET_CONTEXT_COMMAND = "setContext"; |
There was a problem hiding this comment.
This file duplicates the coordinator logic that was also added under packages/vscode-sql-common/src/uriOwnership. Maintaining two implementations will likely diverge and reintroduce coordination bugs. Consider importing and using the shared vscode-sql-common coordinator here (and removing this TODO), or remove the unused shared implementation from this PR.
| ): vscode.CodeLens[] | Thenable<vscode.CodeLens[]> { | ||
| // Defer to notebook-specific code lens provider for notebook cells | ||
| if (document.uri.scheme === "vscode-notebook-cell") { | ||
| if (document.uri.scheme === "vscode-notebook-cell" || uriOwnershipCoordinator?.isOwnedByCoordinatingExtension(document.uri)) { |
There was a problem hiding this comment.
This if condition is long enough that it’s unlikely to match the repo’s Prettier formatting; it should be wrapped to avoid lint/format failures and improve readability.
| if (document.uri.scheme === "vscode-notebook-cell" || uriOwnershipCoordinator?.isOwnedByCoordinatingExtension(document.uri)) { | |
| if ( | |
| document.uri.scheme === "vscode-notebook-cell" || | |
| uriOwnershipCoordinator?.isOwnedByCoordinatingExtension(document.uri) | |
| ) { |
| editor.document.languageId === Constants.languageId && | ||
| uriOwnershipCoordinator?.isOwnedByCoordinatingExtension(editor.document.uri) | ||
| ) { | ||
| this._lastActiveConnectionInfo = undefined; | ||
| } |
There was a problem hiding this comment.
The new _lastActiveConnectionInfo clearing logic for coordinating-extension-owned SQL files should be covered by unit tests (there are already several onDidChangeActiveTextEditor cases in sqlDocumentService.test.ts). Add a case where the URI is reported as owned and assert _lastActiveConnectionInfo is cleared.
| // Don't auto-connect if this document is owned by a coordinating extension. | ||
| if (uriOwnershipCoordinator?.isOwnedByCoordinatingExtension(doc.uri)) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
URI-ownership behavior added here (skipping MSSQL auto-connect when another extension owns the document) isn’t covered by the existing sqlDocumentService unit tests. Please add a test that stubs uriOwnershipCoordinator.isOwnedByCoordinatingExtension(...) to return true and asserts that connect(...) is not called for SQL docs.
| // Defer to notebook-specific code lens provider for notebook cells | ||
| if (document.uri.scheme === "vscode-notebook-cell") { | ||
| if (document.uri.scheme === "vscode-notebook-cell" || uriOwnershipCoordinator?.isOwnedByCoordinatingExtension(document.uri)) { | ||
| return []; | ||
| } |
There was a problem hiding this comment.
provideCodeLenses now suppresses CodeLens when a coordinating extension owns the URI, but the existing SqlCodeLensProvider unit tests don’t cover this branch. Please add a test that stubs uriOwnershipCoordinator.isOwnedByCoordinatingExtension(...) to return true and asserts that no lenses are produced.
| <trans-unit id="mssql.cancelQuery"> | ||
| <source xml:lang="en">Cancel Query</source> | ||
| <source xml:lang="en">Cancel Query (MSSQL)</source> | ||
| </trans-unit> | ||
| <trans-unit id="mssql.changeConnection"> | ||
| <source xml:lang="en">Change Connection</source> | ||
| <source xml:lang="en">Change Connection (MSSQL)</source> |
There was a problem hiding this comment.
These vscode-mssql.xlf source strings were updated, but the corresponding localized XLF files (de/es/fr/it/ja/ko/pt-BR/ru/zh-Hans/zh-Hant) still contain the old <source> text for the same trans-unit IDs. Please update those localized files as well (typically marking targets as state="new") to keep localization in sync.
| // Listen for ownership changes from coordinating extensions | ||
| if (uriOwnershipCoordinator) { | ||
| uriOwnershipCoordinator.onCoordinatingOwnershipChanged(() => { | ||
| const activeEditor = vscode.window.activeTextEditor; |
There was a problem hiding this comment.
The subscription returned by uriOwnershipCoordinator.onCoordinatingOwnershipChanged(...) isn’t disposed, so it will leak listeners across extension reloads/tests. Store the returned Disposable (e.g., in a field) and dispose it in StatusView.dispose() (or register it in a subscription bag).
| // Listen for ownership changes from coordinating extensions | ||
| if (uriOwnershipCoordinator) { | ||
| uriOwnershipCoordinator.onCoordinatingOwnershipChanged(() => { | ||
| const activeEditor = vscode.window.activeTextEditor; |
There was a problem hiding this comment.
New URI-ownership behavior in StatusView (reacting to onCoordinatingOwnershipChanged) isn’t covered by the existing unit tests for this file. Please add a unit test that stubs uriOwnershipCoordinator and verifies the status bar is hidden/shown appropriately when ownership flips.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 14 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| private _refreshCoordinatingExtensions(): void { | ||
| const newExtensions = discoverCoordinatingExtensions(this._context.extension.id); | ||
|
|
||
| for (const extInfo of newExtensions) { | ||
| if (!this._coordinatingExtensionApis.has(extInfo.extensionId)) { | ||
| const extension = vscode.extensions.getExtension(extInfo.extensionId); | ||
| if (extension?.isActive) { | ||
| this._registerCoordinatingExtensionApi(extInfo.extensionId, extension.exports); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| this._coordinatingExtensions = newExtensions; | ||
| } |
There was a problem hiding this comment.
In _refreshCoordinatingExtensions(), when a newly discovered extension is found that hasn't been registered yet, only active extensions are registered (line 246 checks extension?.isActive). However, in the initial discovery path (_discoverAndRegisterExtensions()), inactive extensions are explicitly activated via extension.activate(). The refresh path skips inactive extensions silently, so a newly installed extension that is not yet active at refresh time will not be registered even after it later activates. Consider calling extension.activate() in the refresh path too, consistent with _discoverAndRegisterExtensions().
| { | ||
| "command": "mssql.deployNewDatabase", | ||
| "when": "view == objectExplorer", | ||
| "when": "config.mssql.enableRichExperiences && view == objectExplorer", |
There was a problem hiding this comment.
The mssql.deployNewDatabase menu entry was changed to add config.mssql.enableRichExperiences && to its when clause. However, mssql.enableRichExperiences is not defined as a configuration property in package.json and per the codebase conventions this setting was retired (the correct gate for private preview features is mssql.enableExperimentalFeatures). Since enableRichExperiences is undefined, this condition will always evaluate to false, effectively hiding the "Deploy New Database" button from the Object Explorer for all users. This appears to be an unintended regression introduced by this PR. The condition should either be removed (reverting to "when": "view == objectExplorer") or replaced with config.mssql.enableExperimentalFeatures if it's intended to be feature-gated.
| "when": "config.mssql.enableRichExperiences && view == objectExplorer", | |
| "when": "view == objectExplorer", |
| export class UriOwnershipCoordinator { | ||
| public readonly uriOwnershipApi: UriOwnershipApi; | ||
| public readonly onCoordinatingOwnershipChanged: vscode.Event<void>; | ||
|
|
||
| private readonly _context: vscode.ExtensionContext; | ||
| private readonly _hideUiContextKey: string; | ||
| private readonly _fileOwnedByOtherExtensionMessage?: (extensionName: string) => string; | ||
| private readonly _coordinatingExtensionApis: Map<string, UriOwnershipApi> = new Map(); | ||
| private readonly _coordinatingOwnershipChangedEmitter = new vscode.EventEmitter<void>(); | ||
| private readonly _uriOwnershipChangedEmitter = new vscode.EventEmitter<void>(); | ||
|
|
||
| private _coordinatingExtensions: CoordinatingExtensionInfo[] = []; | ||
| private _ownsUri: ((uri: string) => boolean) | undefined; | ||
| private _releaseUri: ((uri: string) => void | Promise<void>) | undefined; | ||
| private _initialized = false; | ||
|
|
||
| constructor(context: vscode.ExtensionContext, config: UriOwnershipConfig) { | ||
| this._context = context; | ||
| this._hideUiContextKey = config.hideUiContextKey; | ||
| this._fileOwnedByOtherExtensionMessage = config.fileOwnedByOtherExtensionMessage; | ||
|
|
||
| this._context.subscriptions.push(this._coordinatingOwnershipChangedEmitter); | ||
| this._context.subscriptions.push(this._uriOwnershipChangedEmitter); | ||
|
|
||
| this.uriOwnershipApi = { | ||
| ownsUri: (uri: vscode.Uri): boolean => { | ||
| return this._ownsUri?.(uri.toString(true)) ?? false; | ||
| }, | ||
| onDidChangeUriOwnership: this._uriOwnershipChangedEmitter.event, | ||
| }; | ||
|
|
||
| this.onCoordinatingOwnershipChanged = this._coordinatingOwnershipChangedEmitter.event; | ||
|
|
||
| if (config.ownsUri && config.onDidChangeOwnership) { | ||
| this._initializeCallbacks({ | ||
| ownsUri: config.ownsUri, | ||
| onDidChangeOwnership: config.onDidChangeOwnership, | ||
| releaseUri: config.releaseUri, | ||
| }); | ||
| } | ||
|
|
||
| this._discoverAndRegisterExtensions(); | ||
| this._registerActiveEditorListener(); | ||
| this._registerExtensionChangeListener(); | ||
| } | ||
|
|
||
| public initialize(config: UriOwnershipDeferredConfig): void { | ||
| if (this._initialized) { | ||
| return; | ||
| } | ||
| this._initializeCallbacks(config); | ||
| } | ||
|
|
||
| private _initializeCallbacks(config: UriOwnershipDeferredConfig): void { | ||
| if (this._initialized) { | ||
| return; | ||
| } | ||
|
|
||
| this._ownsUri = config.ownsUri; | ||
| this._releaseUri = config.releaseUri; | ||
| this._initialized = true; | ||
|
|
||
| this._context.subscriptions.push( | ||
| config.onDidChangeOwnership(() => { | ||
| this._uriOwnershipChangedEmitter.fire(); | ||
| }), | ||
| ); | ||
|
|
||
| this._updateUriOwnershipContext(); | ||
| } | ||
|
|
||
| public getOwningCoordinatingExtension(uri: vscode.Uri): string | undefined { | ||
| for (const [extensionId, api] of this._coordinatingExtensionApis.entries()) { | ||
| if (api.ownsUri(uri)) { | ||
| return extensionId; | ||
| } | ||
| } | ||
| return undefined; | ||
| } | ||
|
|
||
| public isOwnedByCoordinatingExtension(uri: vscode.Uri): boolean { | ||
| return this.getOwningCoordinatingExtension(uri) !== undefined; | ||
| } | ||
|
|
||
| public isActiveEditorOwnedByOtherExtensionWithWarning(warningMessage?: string): boolean { | ||
| const activeUri = vscode.window.activeTextEditor?.document?.uri; | ||
| if (activeUri) { | ||
| const owningExtensionId = this.getOwningCoordinatingExtension(activeUri); | ||
| if (owningExtensionId) { | ||
| const extensionName = getExtensionDisplayName( | ||
| owningExtensionId, | ||
| this._coordinatingExtensions, | ||
| ); | ||
| const message = | ||
| warningMessage || | ||
| this._fileOwnedByOtherExtensionMessage?.(extensionName) || | ||
| `This file is connected to ${extensionName}. Please use ${extensionName} commands for this file.`; | ||
| void vscode.window.showInformationMessage(message); | ||
| return true; | ||
| } | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| public getCoordinatingExtensions(): ReadonlyArray<CoordinatingExtensionInfo> { | ||
| return this._coordinatingExtensions; | ||
| } | ||
|
|
||
| private _discoverAndRegisterExtensions(): void { | ||
| this._coordinatingExtensions = discoverCoordinatingExtensions(this._context.extension.id); | ||
|
|
||
| for (const extInfo of this._coordinatingExtensions) { | ||
| const extension = vscode.extensions.getExtension(extInfo.extensionId); | ||
| if (!extension) { | ||
| continue; | ||
| } | ||
|
|
||
| if (!extension.isActive) { | ||
| extension.activate().then( | ||
| (exports) => { | ||
| this._registerCoordinatingExtensionApi(extInfo.extensionId, exports); | ||
| }, | ||
| (err) => { | ||
| console.error( | ||
| `[${this._context.extension.id}] Error activating coordinating extension ${extInfo.extensionId}: ${err}`, | ||
| ); | ||
| }, | ||
| ); | ||
| } else { | ||
| this._registerCoordinatingExtensionApi(extInfo.extensionId, extension.exports); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private _registerCoordinatingExtensionApi(extensionId: string, exports: unknown): void { | ||
| const api = (exports as { uriOwnershipApi?: UriOwnershipApi })?.uriOwnershipApi; | ||
| if (api) { | ||
| this._coordinatingExtensionApis.set(extensionId, api); | ||
|
|
||
| if (api.onDidChangeUriOwnership) { | ||
| this._context.subscriptions.push( | ||
| api.onDidChangeUriOwnership(() => { | ||
| this._updateUriOwnershipContext(); | ||
| }), | ||
| ); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private _registerActiveEditorListener(): void { | ||
| this._context.subscriptions.push( | ||
| vscode.window.onDidChangeActiveTextEditor(() => { | ||
| this._updateUriOwnershipContext(); | ||
| }), | ||
| ); | ||
|
|
||
| this._updateUriOwnershipContext(); | ||
| } | ||
|
|
||
| private _registerExtensionChangeListener(): void { | ||
| this._context.subscriptions.push( | ||
| vscode.extensions.onDidChange(() => { | ||
| this._refreshCoordinatingExtensions(); | ||
| }), | ||
| ); | ||
| } | ||
|
|
||
| private _refreshCoordinatingExtensions(): void { | ||
| const newExtensions = discoverCoordinatingExtensions(this._context.extension.id); | ||
|
|
||
| for (const extInfo of newExtensions) { | ||
| if (!this._coordinatingExtensionApis.has(extInfo.extensionId)) { | ||
| const extension = vscode.extensions.getExtension(extInfo.extensionId); | ||
| if (extension?.isActive) { | ||
| this._registerCoordinatingExtensionApi(extInfo.extensionId, extension.exports); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| this._coordinatingExtensions = newExtensions; | ||
| } | ||
|
|
||
| private _updateUriOwnershipContext(): void { | ||
| const activeEditor = vscode.window.activeTextEditor; | ||
| if (!activeEditor) { | ||
| void vscode.commands.executeCommand(SET_CONTEXT_COMMAND, this._hideUiContextKey, false); | ||
| return; | ||
| } | ||
|
|
||
| const uri = activeEditor.document.uri; | ||
| const uriString = uri.toString(true); | ||
| const isOwnedByOther = this.isOwnedByCoordinatingExtension(uri); | ||
| const isOwnedBySelf = this._ownsUri?.(uriString) ?? false; | ||
|
|
||
| if (isOwnedByOther && isOwnedBySelf && this._releaseUri) { | ||
| void Promise.resolve(this._releaseUri(uriString)); | ||
| } | ||
|
|
||
| void vscode.commands.executeCommand( | ||
| SET_CONTEXT_COMMAND, | ||
| this._hideUiContextKey, | ||
| isOwnedByOther, | ||
| ); | ||
|
|
||
| this._coordinatingOwnershipChangedEmitter.fire(); | ||
| } | ||
| } |
There was a problem hiding this comment.
The UriOwnershipCoordinator class in uriOwnershipCore.ts has no unit tests, despite the project having comprehensive unit test coverage for similar subsystems (e.g., sqlCodeLensProvider.test.ts, sqlDocumentService.test.ts, statusView.test.ts). Key behaviors that could benefit from test coverage include: isOwnedByCoordinatingExtension(), conflict resolution in _updateUriOwnershipContext() (the isOwnedByOther && isOwnedBySelf && _releaseUri branch), discoverCoordinatingExtensions() extension filtering, and initialize() being a no-op after first call.
| private _updateUriOwnershipContext(): void { | ||
| const activeEditor = vscode.window.activeTextEditor; | ||
| if (!activeEditor) { | ||
| void vscode.commands.executeCommand(SET_CONTEXT_COMMAND, this._hideUiContextKey, false); | ||
| return; | ||
| } | ||
|
|
||
| const uri = activeEditor.document.uri; | ||
| const uriString = uri.toString(true); | ||
| const isOwnedByOther = this.isOwnedByCoordinatingExtension(uri); | ||
| const isOwnedBySelf = this._ownsUri?.(uriString) ?? false; | ||
|
|
||
| if (isOwnedByOther && isOwnedBySelf && this._releaseUri) { | ||
| void Promise.resolve(this._releaseUri(uriString)); | ||
| } | ||
|
|
||
| void vscode.commands.executeCommand( | ||
| SET_CONTEXT_COMMAND, | ||
| this._hideUiContextKey, | ||
| isOwnedByOther, | ||
| ); | ||
|
|
||
| this._coordinatingOwnershipChangedEmitter.fire(); |
There was a problem hiding this comment.
The _updateUriOwnershipContext() method unconditionally fires _coordinatingOwnershipChangedEmitter every time it is called — including on every active editor change (vscode.window.onDidChangeActiveTextEditor). This means onCoordinatingOwnershipChanged fires on every tab switch, triggering unnecessary work in all subscribers (StatusView's handler, SqlCodeLensProvider, etc.) even when the ownership state hasn't changed. The emitter should only be fired when the isOwnedByOther value actually changes, i.e. when transitioning from owned-by-other to not-owned-by-other or vice versa. This can be achieved by caching the previous value and only emitting when it changes.
| private _updateUriOwnershipContext(): void { | |
| const activeEditor = vscode.window.activeTextEditor; | |
| if (!activeEditor) { | |
| void vscode.commands.executeCommand(SET_CONTEXT_COMMAND, this._hideUiContextKey, false); | |
| return; | |
| } | |
| const uri = activeEditor.document.uri; | |
| const uriString = uri.toString(true); | |
| const isOwnedByOther = this.isOwnedByCoordinatingExtension(uri); | |
| const isOwnedBySelf = this._ownsUri?.(uriString) ?? false; | |
| if (isOwnedByOther && isOwnedBySelf && this._releaseUri) { | |
| void Promise.resolve(this._releaseUri(uriString)); | |
| } | |
| void vscode.commands.executeCommand( | |
| SET_CONTEXT_COMMAND, | |
| this._hideUiContextKey, | |
| isOwnedByOther, | |
| ); | |
| this._coordinatingOwnershipChangedEmitter.fire(); | |
| private _lastIsOwnedByOther: boolean | undefined; | |
| private _updateUriOwnershipContext(): void { | |
| const activeEditor = vscode.window.activeTextEditor; | |
| let isOwnedByOther = false; | |
| if (!activeEditor) { | |
| void vscode.commands.executeCommand(SET_CONTEXT_COMMAND, this._hideUiContextKey, false); | |
| } else { | |
| const uri = activeEditor.document.uri; | |
| const uriString = uri.toString(true); | |
| isOwnedByOther = this.isOwnedByCoordinatingExtension(uri); | |
| const isOwnedBySelf = this._ownsUri?.(uriString) ?? false; | |
| if (isOwnedByOther && isOwnedBySelf && this._releaseUri) { | |
| void Promise.resolve(this._releaseUri(uriString)); | |
| } | |
| void vscode.commands.executeCommand( | |
| SET_CONTEXT_COMMAND, | |
| this._hideUiContextKey, | |
| isOwnedByOther, | |
| ); | |
| } | |
| if (this._lastIsOwnedByOther !== isOwnedByOther) { | |
| this._lastIsOwnedByOther = isOwnedByOther; | |
| this._coordinatingOwnershipChangedEmitter.fire(); | |
| } |
|
@aasimkhan30 this might be a good PR to discuss in a design meeting. It's like to make sure I understand the details of what we're doing here. Thanks! |
Description
Fixes: #19535
This PR implements a URI ownership coordination system between the MSSQL and PostgreSQL VS Code extensions. When both extensions are installed, they now coordinate to ensure only one extension shows UI elements (status bar, CodeLens) for a given SQL file at a time, based on which extension has an active connection to that file.
Problem
When both MSSQL and PostgreSQL extensions are installed, users experienced:
Solution
Implemented a
UriOwnershipCoordinatorthat:uriOwnershipApi) for other extensions to query URI ownershipBehavior
User-Facing Messages
When a user tries to connect or run a query on a file connected to PostgreSQL through commands:
Backward Compatibility
uriOwnershipApi(older version)Code Changes Checklist
npm run test)Reviewers: Please read our reviewer guidelines