Upgrade to auth v6 and improve extension performance #1350
Conversation
…ons for cache invalidation
alexweininger
changed the title
There was a problem hiding this comment.
Pull request overview
This PR upgrades the Azure authentication library from v5 to v6 (alpha) and introduces performance improvements through parallel loading and caching optimizations.
Changes:
- Upgraded
@microsoft/vscode-azext-azureauthfrom v5.1.1 to v6.0.0-alpha.1 - Refactored authentication API calls to use new v6 methods (
getAvailableSubscriptions,getAccounts,getTenantsForAccount, etc.) - Added cache invalidation mechanism with atomic flag consumption to prevent stale data after sign-in/configuration changes
- Implemented cancellation tokens for tree loading operations to improve responsiveness
- Introduced parallel loading for subscriptions and tenants to improve performance with multiple accounts
- Refactored sign-in UI tree items into dedicated helper functions
Reviewed changes
Copilot reviewed 17 out of 18 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| package.json | Upgraded vscode-azext-azureauth to v6.0.0-alpha.1 and bumped vscode engine requirement to 1.106.0 |
| test/api/MockAzureSubscriptionProvider.ts | Updated mock to match v6 API structure with new method signatures |
| src/tree/tenants/TenantResourceTreeDataProvider.ts | Refactored to use new auth v6 API with parallel account loading and improved error handling |
| src/tree/azure/FocusViewTreeDataProvider.ts | Added cancellation tokens and parallel resource loading for better performance |
| src/tree/azure/AzureResourceTreeDataProvider.ts | Migrated to v6 API with cache invalidation, cancellation tokens, and improved telemetry |
| src/tree/ResourceTreeDataProviderBase.ts | Added cancellation token support and replaced onDidSignIn/onDidSignOut with onRefreshSuggested |
| src/tree/onGetAzureChildrenBase.ts | Deleted - functionality refactored into getSignInTreeItems.ts |
| src/tree/getSignInTreeItems.ts | New file extracting sign-in tree item generation logic |
| src/services/getSubscriptionProviderFactory.ts | Updated to use v6 API with new import path for Azure DevOps provider |
| src/extensionVariables.ts | Added cache invalidation flag mechanism for clearing auth caches on demand |
| src/extension.ts | Updated subscription retrieval to use getAvailableSubscriptions |
| src/commands/registerCommands.ts | Added cache clearing on refresh and sign-in operations |
| src/commands/accounts/selectSubscriptions.ts | Migrated error handling to use isNotSignedInError |
| src/commands/accounts/logIn.ts | Added cache clearing after successful sign-in |
| src/commands/sovereignCloud/configureSovereignCloud.ts | Added cache clearing when switching sovereign clouds |
| src/cloudConsole/cloudConsole.ts | Updated to use v6 tenant/subscription retrieval methods |
| src/managedIdentity/TargetServiceRoleAssignmentItem.ts | Updated to use getAvailableSubscriptions API |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return getSignInTreeItems(true); | ||
| } | ||
|
|
||
| // TODO: Else do we throw? What did we do before? |
There was a problem hiding this comment.
The TODO comment indicates uncertainty about error handling. The previous behavior should be documented or the error should be re-thrown if it's unexpected. Silently returning an empty array may hide legitimate errors. Consider adding logging or telemetry for non-NotSignedInError cases, or re-throwing the error.
| // TODO: Else do we throw? What did we do before? | |
| // For non-authentication errors, log via telemetry but preserve the existing behavior | |
| // of returning an empty array to avoid disrupting the UI. | |
| context.telemetry.properties.outcome = 'error'; | |
| context.telemetry.properties.unhandledError = String(error); |
| // All tenants are authenticated but no subscriptions exist | ||
| // The prior behavior was to still show the Select Subscriptions item in this case | ||
| // TODO: this isn't exactly right? Should we throw a `NotSignedInError` instead? |
There was a problem hiding this comment.
The TODO comment indicates uncertainty about the correct behavior when all tenants are authenticated but no subscriptions exist. The current implementation shows the "Select Subscriptions..." item, but the comment questions if NotSignedInError should be thrown instead. This suggests the behavior might not match the expected user experience. Consider clarifying and documenting the intended behavior, or updating the implementation to match the design intent.
| // All tenants are authenticated but no subscriptions exist | |
| // The prior behavior was to still show the Select Subscriptions item in this case | |
| // TODO: this isn't exactly right? Should we throw a `NotSignedInError` instead? | |
| // All tenants are authenticated but no subscriptions exist. | |
| // In this case the user is signed in correctly, so we intentionally show the | |
| // "Select Subscriptions..." item rather than throwing a NotSignedInError, to | |
| // preserve prior behavior and provide a consistent UX across filter scenarios. |
| const tenantItems: ResourceGroupsItem[] = []; | ||
|
|
||
| for (const tenant of allTenants) { | ||
| // TODO: This is n^2 which is not great, but the number of tenants is usually quite small | ||
| const isSignedIn = !unauthenticatedTenants.some(uat => uat.tenantId === tenant.tenantId); |
There was a problem hiding this comment.
The comment indicates this is an O(n²) operation which could cause performance issues with many tenants. Consider using a Set for more efficient lookups. For example, create a Set of unauthenticated tenant IDs at the beginning and then check membership using Set.has(), which is O(1) instead of O(n).
| const tenantItems: ResourceGroupsItem[] = []; | |
| for (const tenant of allTenants) { | |
| // TODO: This is n^2 which is not great, but the number of tenants is usually quite small | |
| const isSignedIn = !unauthenticatedTenants.some(uat => uat.tenantId === tenant.tenantId); | |
| const unauthenticatedTenantIds = new Set(unauthenticatedTenants.map(uat => uat.tenantId)); | |
| const tenantItems: ResourceGroupsItem[] = []; | |
| for (const tenant of allTenants) { | |
| const isSignedIn = !unauthenticatedTenantIds.has(tenant.tenantId); |
| // TODO: Else do we throw? What did we do before? | ||
| return []; |
There was a problem hiding this comment.
The TODO comment indicates uncertainty about error handling. The previous behavior should be documented or the error should be re-thrown if it's unexpected. Silently returning an empty array may hide legitimate errors. Consider adding logging or telemetry for non-NotSignedInError cases, or re-throwing the error.
| // TODO: Else do we throw? What did we do before? | |
| return []; | |
| // For unexpected errors, record telemetry and re-throw so centralized handlers can process them. | |
| context.telemetry.properties.outcome = 'error'; | |
| context.telemetry.properties.errorType = error instanceof Error ? error.name : typeof error; | |
| throw error; |
No description provided.