Skip to content

Commit b7ecc73

Browse files
committed
Address Brandon's PR feedback: reduce logging, use context.ui, add l10n
- Remove root-level package-lock.json (unnecessary) - Reduce excessive logging in RevealStep.ts - Remove verbose appendLog calls throughout - Use .debug() and .error() for appropriate log levels - Remove unnecessary extension activation (Resources extension handles it) - Update addService.ts to use context.ui.showInputBox/showQuickPick - Add vscode.l10n.t() to all user-facing strings - Update extensions.ts to use context.ui.showInputBox - Update Copilot instructions with UI best practices - Document l10n requirement for user-facing strings - Document context.ui usage instead of vscode.window - Note FileSystemWatcher scarcity
1 parent 52ed9e8 commit b7ecc73

File tree

5 files changed

+33
-108
lines changed

5 files changed

+33
-108
lines changed

ext/vscode/.github/copilot-instructions.md

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -172,6 +172,14 @@ When working on `azure.yaml` language support in `src/language/`:
172172
- Use telemetry to measure and track performance metrics
173173
- Follow the patterns in `src/views/` for efficient tree providers
174174

175+
### User Interface Best Practices
176+
- All user-facing strings shown in the UI, error messages, etc. must use `vscode.l10n.t()`
177+
- All user-facing strings in package.json must be extracted into package.nls.json
178+
- Instead of `vscode.window.showQuickPick`, use `IActionContext.ui.showQuickPick`
179+
- Instead of `vscode.window.showInputBox`, use `IActionContext.ui.showInputBox`
180+
- The same applies for `showWarningMessage`, `showOpenDialog`, and `showWorkspaceFolderPick`
181+
- FileSystemWatchers are a scarce resource on some systems - consolidate into shared watchers when possible
182+
175183
## Build & Package
176184
- Development build: `npm run dev-build`
177185
- Production build: `npm run build`

ext/vscode/src/commands/addService.ts

Lines changed: 17 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -22,72 +22,60 @@ export async function addService(context: IActionContext, node?: AzureDevCliMode
2222
if (!documentUri) {
2323
const workspaceFolders = vscode.workspace.workspaceFolders;
2424
if (!workspaceFolders || workspaceFolders.length === 0) {
25-
void vscode.window.showErrorMessage('No workspace folder is open.');
25+
void vscode.window.showErrorMessage(vscode.l10n.t('No workspace folder is open.'));
2626
return;
2727
}
2828

2929
// Search for azure.yaml or azure.yml files in workspace
3030
const azureYamlFiles = await vscode.workspace.findFiles('**/azure.{yml,yaml}', '**/node_modules/**', 1);
3131
if (azureYamlFiles.length === 0) {
32-
void vscode.window.showErrorMessage('No azure.yaml file found in workspace.');
32+
void vscode.window.showErrorMessage(vscode.l10n.t('No azure.yaml file found in workspace.'));
3333
return;
3434
}
3535

3636
documentUri = azureYamlFiles[0];
3737
}
3838

3939
// Prompt for service name
40-
const serviceName = await vscode.window.showInputBox({
41-
prompt: 'Enter service name',
40+
const serviceName = await context.ui.showInputBox({
41+
prompt: vscode.l10n.t('Enter service name'),
4242
placeHolder: 'api',
4343
validateInput: (value) => {
4444
if (!value || !/^[a-zA-Z0-9-_]+$/.test(value)) {
45-
return 'Service name must contain only letters, numbers, hyphens, and underscores';
45+
return vscode.l10n.t('Service name must contain only letters, numbers, hyphens, and underscores');
4646
}
4747
return undefined;
4848
}
4949
});
5050

51-
if (!serviceName) {
52-
return;
53-
}
54-
5551
// Prompt for programming language
56-
const language = await vscode.window.showQuickPick(
57-
['python', 'js', 'ts', 'csharp', 'java', 'go'],
58-
{ placeHolder: 'Select programming language' }
52+
const language = await context.ui.showQuickPick(
53+
['python', 'js', 'ts', 'csharp', 'java', 'go'].map(lang => ({ label: lang })),
54+
{ placeHolder: vscode.l10n.t('Select programming language') }
5955
);
6056

61-
if (!language) {
62-
return;
63-
}
64-
6557
// Prompt for Azure host
66-
const host = await vscode.window.showQuickPick(
58+
const host = await context.ui.showQuickPick(
6759
[
68-
{ label: 'containerapp', description: 'Azure Container Apps' },
69-
{ label: 'appservice', description: 'Azure App Service' },
70-
{ label: 'function', description: 'Azure Functions' }
60+
{ label: 'containerapp', description: vscode.l10n.t('Azure Container Apps') },
61+
{ label: 'appservice', description: vscode.l10n.t('Azure App Service') },
62+
{ label: 'function', description: vscode.l10n.t('Azure Functions') }
7163
],
72-
{ placeHolder: 'Select Azure host' }
64+
{ placeHolder: vscode.l10n.t('Select Azure host') }
7365
);
7466

75-
if (!host) {
76-
return;
77-
}
78-
7967
try {
8068
const document = await vscode.workspace.openTextDocument(documentUri);
8169
const text = document.getText();
8270
const doc = yaml.parseDocument(text);
8371

8472
const services = doc.get('services') as yaml.YAMLMap;
8573
if (!services) {
86-
void vscode.window.showErrorMessage('No services section found in azure.yaml');
74+
void vscode.window.showErrorMessage(vscode.l10n.t('No services section found in azure.yaml'));
8775
return;
8876
}
8977

90-
const serviceSnippet = `\n ${serviceName}:\n project: ./${serviceName}\n language: ${language}\n host: ${host.label}`;
78+
const serviceSnippet = `\n ${serviceName}:\n project: ./${serviceName}\n language: ${language.label}\n host: ${host.label}`;
9179

9280
// Find the end of the services section
9381
if (doc.contents && yaml.isMap(doc.contents)) {
@@ -99,11 +87,11 @@ export async function addService(context: IActionContext, node?: AzureDevCliMode
9987
const success = await vscode.workspace.applyEdit(edit);
10088

10189
if (success) {
102-
void vscode.window.showInformationMessage(`Service '${serviceName}' added to azure.yaml`);
90+
void vscode.window.showInformationMessage(vscode.l10n.t('Service \'{0}\' added to azure.yaml', serviceName));
10391
}
10492
}
10593
}
10694
} catch (error) {
107-
void vscode.window.showErrorMessage(`Failed to add service: ${error instanceof Error ? error.message : String(error)}`);
95+
void vscode.window.showErrorMessage(vscode.l10n.t('Failed to add service: {0}', error instanceof Error ? error.message : String(error)));
10896
}
10997
}

ext/vscode/src/commands/azureWorkspace/wizard/RevealStep.ts

Lines changed: 5 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -12,116 +12,66 @@ export class RevealStep extends AzureWizardExecuteStep<RevealResourceWizardConte
1212
public readonly priority: number = 100;
1313

1414
public shouldExecute(wizardContext: RevealResourceWizardContext | RevealResourceGroupWizardContext): boolean {
15-
const should = !!wizardContext.azureResourceId;
16-
ext.outputChannel.appendLog(vscode.l10n.t('RevealStep shouldExecute: {0}, azureResourceId: {1}', should, wizardContext.azureResourceId || 'undefined'));
17-
return should;
15+
return !!wizardContext.azureResourceId;
1816
}
1917

2018
public async execute(context: RevealResourceWizardContext | RevealResourceGroupWizardContext): Promise<void> {
21-
ext.outputChannel.appendLog(vscode.l10n.t('RevealStep starting execute with azureResourceId: {0}', context.azureResourceId || 'undefined'));
2219
const azureResourceId = nonNullProp(context, 'azureResourceId');
23-
ext.outputChannel.appendLog(vscode.l10n.t('Getting Azure Resource Extension API...'));
2420
const api = await getAzureResourceExtensionApi();
25-
ext.outputChannel.appendLog(vscode.l10n.t('API obtained, focusing Azure Resources view...'));
2621

2722
// Show the Azure Resources view first to ensure the reveal is visible
2823
await vscode.commands.executeCommand('azureResourceGroups.focus');
29-
ext.outputChannel.appendLog(vscode.l10n.t('View focused'));
30-
31-
// Extract provider from resource ID to determine which extension to activate
32-
const providerMatch = azureResourceId.match(/\/providers\/([^/]+)/i);
33-
const provider = providerMatch ? providerMatch[1] : null;
34-
ext.outputChannel.appendLog(vscode.l10n.t('Resource provider: {0}', provider || 'none'));
35-
36-
// Activate the appropriate Azure extension based on provider
37-
if (provider) {
38-
const extensionMap: Record<string, string> = {
39-
// eslint-disable-next-line @typescript-eslint/naming-convention
40-
'Microsoft.App': 'ms-azuretools.vscode-azurecontainerapps',
41-
// eslint-disable-next-line @typescript-eslint/naming-convention
42-
'Microsoft.Web': 'ms-azuretools.vscode-azurefunctions',
43-
// eslint-disable-next-line @typescript-eslint/naming-convention
44-
'Microsoft.Storage': 'ms-azuretools.vscode-azurestorage',
45-
// eslint-disable-next-line @typescript-eslint/naming-convention
46-
'Microsoft.DocumentDB': 'ms-azuretools.azure-cosmos',
47-
};
48-
49-
const extensionId = extensionMap[provider];
50-
if (extensionId) {
51-
ext.outputChannel.appendLog(vscode.l10n.t('Activating extension: {0}', extensionId));
52-
const extension = vscode.extensions.getExtension(extensionId);
53-
if (extension && !extension.isActive) {
54-
await extension.activate();
55-
ext.outputChannel.appendLog(vscode.l10n.t('Extension activated'));
56-
// Delay to allow the extension to register its tree data provider.
57-
// The Azure Resources API doesn't provide an event for when provider registration completes.
58-
await new Promise(resolve => setTimeout(resolve, 1000));
59-
}
60-
}
61-
}
62-
63-
ext.outputChannel.appendLog(vscode.l10n.t('Attempting reveal...'));
6424

6525
try {
6626
// Try to refresh the Azure Resources view to ensure the tree is loaded
67-
ext.outputChannel.appendLog(vscode.l10n.t('Refreshing Azure Resources tree...'));
6827
try {
6928
await vscode.commands.executeCommand('azureResourceGroups.refresh');
70-
ext.outputChannel.appendLog(vscode.l10n.t('Refresh command executed'));
7129
// Delay to allow the tree to fully populate after refresh.
7230
// The Azure Resources API doesn't expose an event for tree load completion,
7331
// so we use a delay as a pragmatic workaround.
7432
await new Promise(resolve => setTimeout(resolve, 1500));
7533
} catch (refreshError) {
76-
ext.outputChannel.appendLog(vscode.l10n.t('Refresh command not available or failed: {0}', refreshError instanceof Error ? refreshError.message : String(refreshError)));
34+
ext.outputChannel.debug(vscode.l10n.t('Refresh command not available or failed: {0}', refreshError instanceof Error ? refreshError.message : String(refreshError)));
7735
}
7836

7937
// Extract subscription and resource group from the resource ID to reveal the RG first
8038
const resourceIdMatch = azureResourceId.match(/\/subscriptions\/([^/]+)\/resourceGroups\/([^/]+)/i);
8139
if (resourceIdMatch) {
8240
const subscriptionId = resourceIdMatch[1];
8341
const resourceGroupName = resourceIdMatch[2];
84-
ext.outputChannel.appendLog(vscode.l10n.t('Subscription: {0}, Resource Group: {1}', subscriptionId, resourceGroupName));
8542

8643
// Try revealing the resource group first to ensure the tree is expanded
8744
const rgResourceId = `/subscriptions/${subscriptionId}/resourceGroups/${resourceGroupName}`;
88-
ext.outputChannel.appendLog(vscode.l10n.t('Revealing resource group first: {0}', rgResourceId));
8945
try {
9046
await api.resources.revealAzureResource(rgResourceId, { select: false, focus: false, expand: true });
9147
// Delay to allow the tree node to expand before revealing the child resource.
9248
// The revealAzureResource API returns before the tree UI fully updates.
9349
await new Promise(resolve => setTimeout(resolve, 1000));
9450
} catch (rgError) {
95-
ext.outputChannel.appendLog(vscode.l10n.t('Resource group reveal failed: {0}', rgError instanceof Error ? rgError.message : String(rgError)));
51+
ext.outputChannel.debug(vscode.l10n.t('Resource group reveal failed: {0}', rgError instanceof Error ? rgError.message : String(rgError)));
9652
}
9753
}
9854

99-
ext.outputChannel.appendLog(vscode.l10n.t('Calling revealAzureResource with options: select=true, focus=true, expand=true'));
10055
const result = await api.resources.revealAzureResource(azureResourceId, { select: true, focus: true, expand: true });
101-
ext.outputChannel.appendLog(vscode.l10n.t('revealAzureResource returned: {0}', String(result)));
10256

10357
// Note: The focusGroup command to trigger "Focused Resources" view requires internal
10458
// tree item context that's not accessible through the public API. Users can manually
10559
// click the zoom-in icon on the resource group if they want the focused view.
10660

10761
// Try a second time if needed
10862
if (result === undefined) {
109-
ext.outputChannel.appendLog(vscode.l10n.t('First reveal returned undefined, trying again after delay...'));
11063
// Retry delay: the first reveal may fail if the tree hasn't finished loading.
11164
// A brief delay before retry often succeeds where the first attempt failed.
11265
await new Promise(resolve => setTimeout(resolve, 1000));
11366
const secondResult = await api.resources.revealAzureResource(azureResourceId, { select: true, focus: true, expand: true });
114-
ext.outputChannel.appendLog(vscode.l10n.t('Second attempt returned: {0}', String(secondResult)));
11567

11668
// Try using the openInPortal command as an alternative
11769
if (secondResult === undefined) {
118-
ext.outputChannel.appendLog(vscode.l10n.t('Reveal API not working as expected, trying alternative approach'));
11970
// Try the workspace resource reveal command specific to this view
12071
try {
12172
await vscode.commands.executeCommand('azureResourceGroups.revealResource', azureResourceId);
122-
ext.outputChannel.appendLog(vscode.l10n.t('Alternative reveal command succeeded'));
12373
} catch (altError) {
124-
ext.outputChannel.appendLog(vscode.l10n.t('Alternative reveal also failed: {0}', altError instanceof Error ? altError.message : String(altError)));
74+
ext.outputChannel.debug(vscode.l10n.t('Alternative reveal also failed: {0}', altError instanceof Error ? altError.message : String(altError)));
12575
vscode.window.showInformationMessage(
12676
vscode.l10n.t('Unable to automatically reveal resource in tree. Resource ID: {0}', azureResourceId),
12777
vscode.l10n.t('Copy Resource ID'),
@@ -136,11 +86,8 @@ export class RevealStep extends AzureWizardExecuteStep<RevealResourceWizardConte
13686
}
13787
}
13888
}
139-
140-
ext.outputChannel.appendLog(vscode.l10n.t('revealAzureResource completed'));
14189
} catch (error) {
142-
ext.outputChannel.appendLog(vscode.l10n.t('Failed to reveal resource: {0}', error instanceof Error ? error.message : String(error)));
143-
// Show error to user
90+
ext.outputChannel.error(vscode.l10n.t('Failed to reveal resource: {0}', error instanceof Error ? error.message : String(error)));
14491
vscode.window.showErrorMessage(vscode.l10n.t('Failed to reveal Azure resource: {0}', error instanceof Error ? error.message : String(error)));
14592
throw error;
14693
}

ext/vscode/src/commands/extensions.ts

Lines changed: 3 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -71,15 +71,11 @@ export async function installExtension(context: IActionContext): Promise<void> {
7171
await runExtensionCommand(context, args, vscode.l10n.t('Adding extension source...'), TelemetryId.ExtensionSourceAddCli);
7272
}
7373

74-
const id = await vscode.window.showInputBox({
74+
const id = await context.ui.showInputBox({
7575
prompt: vscode.l10n.t('Enter the ID of the extension to install'),
7676
placeHolder: vscode.l10n.t('Extension ID')
7777
});
7878

79-
if (!id) {
80-
return;
81-
}
82-
8379
const args = composeArgs(
8480
withArg('extension', 'install'),
8581
withQuotedArg(id)
@@ -92,16 +88,12 @@ export async function uninstallExtension(context: IActionContext, item?: Extensi
9288
let id = item?.extension.id;
9389

9490
if (!id) {
95-
id = await vscode.window.showInputBox({
91+
id = await context.ui.showInputBox({
9692
prompt: vscode.l10n.t('Enter the ID of the extension to uninstall'),
9793
placeHolder: vscode.l10n.t('Extension ID')
9894
});
9995
}
10096

101-
if (!id) {
102-
return;
103-
}
104-
10597
const args = composeArgs(
10698
withArg('extension', 'uninstall'),
10799
withQuotedArg(id)
@@ -114,16 +106,12 @@ export async function upgradeExtension(context: IActionContext, item?: Extension
114106
let id = item?.extension.id;
115107

116108
if (!id) {
117-
id = await vscode.window.showInputBox({
109+
id = await context.ui.showInputBox({
118110
prompt: vscode.l10n.t('Enter the ID of the extension to upgrade'),
119111
placeHolder: vscode.l10n.t('Extension ID')
120112
});
121113
}
122114

123-
if (!id) {
124-
return;
125-
}
126-
127115
const args = composeArgs(
128116
withArg('extension', 'install'),
129117
withQuotedArg(id),

package-lock.json

Lines changed: 0 additions & 6 deletions
This file was deleted.

0 commit comments

Comments
 (0)