From 44947b43a3328e0999bda23cfcf08d31ec882bbe Mon Sep 17 00:00:00 2001 From: Dave <3836813+enmande@users.noreply.github.com> Date: Tue, 27 Jan 2026 14:27:13 -0500 Subject: [PATCH] feat(account-switching) [PM-5594]: Enabling account switching causes performance issues on safari (#18339) * refactor(account-switching) [PM-5594]: Move account switching enabled flag to AccountSwitcherService to accommodate server-side feature flag. * test(account-switching) [PM-5594]: Update tests to include ConfigService dependency for feature flag. * refactor(account-switching) [PM-5594]: Remove compile-time account switching flags from browser. * refactor(account-switching) [PM-5594]: Move initialization to ctor for strict. --- apps/browser/config/base.json | 1 - apps/browser/config/development.json | 3 --- apps/browser/config/production.json | 5 ---- .../account-switcher.component.html | 9 ++++--- .../account-switcher.component.ts | 27 ++++++++++--------- .../services/account-switcher.service.spec.ts | 3 +++ .../services/account-switcher.service.ts | 18 +++++++++++++ .../settings/excluded-domains.component.html | 4 ++- .../settings/excluded-domains.component.ts | 12 ++++----- apps/browser/src/platform/flags.ts | 17 +----------- libs/common/src/enums/feature-flag.enum.ts | 2 ++ 11 files changed, 54 insertions(+), 47 deletions(-) delete mode 100644 apps/browser/config/production.json diff --git a/apps/browser/config/base.json b/apps/browser/config/base.json index 02bdc5d22af..0de1da0a648 100644 --- a/apps/browser/config/base.json +++ b/apps/browser/config/base.json @@ -1,7 +1,6 @@ { "devFlags": {}, "flags": { - "accountSwitching": false, "sdk": true } } diff --git a/apps/browser/config/development.json b/apps/browser/config/development.json index 042a98c2c39..12a34d8cbee 100644 --- a/apps/browser/config/development.json +++ b/apps/browser/config/development.json @@ -4,8 +4,5 @@ "base": "https://localhost:8080" }, "skipWelcomeOnInstall": true - }, - "flags": { - "accountSwitching": true } } diff --git a/apps/browser/config/production.json b/apps/browser/config/production.json deleted file mode 100644 index a43eee1d5c9..00000000000 --- a/apps/browser/config/production.json +++ /dev/null @@ -1,5 +0,0 @@ -{ - "flags": { - "accountSwitching": true - } -} diff --git a/apps/browser/src/auth/popup/account-switching/account-switcher.component.html b/apps/browser/src/auth/popup/account-switching/account-switcher.component.html index cef2a748d58..0a9e2a1dd9d 100644 --- a/apps/browser/src/auth/popup/account-switching/account-switcher.component.html +++ b/apps/browser/src/auth/popup/account-switching/account-switcher.component.html @@ -7,13 +7,16 @@ - + -
+
- +

{{ "availableAccounts" | i18n }}

 diff --git a/apps/browser/src/auth/popup/account-switching/account-switcher.component.ts b/apps/browser/src/auth/popup/account-switching/account-switcher.component.ts index d7d3c02ab14..ae7f66a9018 100644 --- a/apps/browser/src/auth/popup/account-switching/account-switcher.component.ts +++ b/apps/browser/src/auth/popup/account-switching/account-switcher.component.ts @@ -1,7 +1,7 @@ import { CommonModule, Location } from "@angular/common"; import { Component, OnDestroy, OnInit } from "@angular/core"; import { Router } from "@angular/router"; -import { Subject, firstValueFrom, map, of, startWith, switchMap } from "rxjs"; +import { Observable, Subject, firstValueFrom, map, of, startWith, switchMap } from "rxjs"; import { JslibModule } from "@bitwarden/angular/jslib.module"; import { LockService, LogoutService } from "@bitwarden/auth/common"; @@ -24,7 +24,6 @@ import { TypographyModule, } from "@bitwarden/components"; -import { enableAccountSwitching } from "../../../platform/flags"; import { PopOutComponent } from "../../../platform/popup/components/pop-out.component"; import { PopupHeaderComponent } from "../../../platform/popup/layout/popup-header.component"; import { PopupPageComponent } from "../../../platform/popup/layout/popup-page.component"; @@ -59,7 +58,7 @@ export class AccountSwitcherComponent implements OnInit, OnDestroy { loading = false; activeUserCanLock = false; - enableAccountSwitching = true; + enableAccountSwitching$: Observable; constructor( private accountSwitcherService: AccountSwitcherService, @@ -72,7 +71,9 @@ export class AccountSwitcherComponent implements OnInit, OnDestroy { private authService: AuthService, private lockService: LockService, private logoutService: LogoutService, - ) {} + ) { + this.enableAccountSwitching$ = this.accountSwitcherService.accountSwitchingEnabled$(); + } get accountLimit() { return this.accountSwitcherService.ACCOUNT_LIMIT; @@ -97,19 +98,21 @@ export class AccountSwitcherComponent implements OnInit, OnDestroy { switchMap((accounts) => { // If account switching is disabled, don't show the lock all button // as only one account should be shown. - if (!enableAccountSwitching()) { - return of(false); - } + return this.accountSwitcherService.accountSwitchingEnabled$().pipe( + switchMap((enabled) => { + if (!enabled) { + return of(false); + } - // When there are an inactive accounts provide the option to lock all accounts - // Note: "Add account" is counted as an inactive account, so check for more than one account - return of(accounts.length > 1); + // When there are inactive accounts provide the option to lock all accounts + // Note: "Add account" is counted as an inactive account, so check for more than one account + return of(accounts.length > 1); + }), + ); }), ); async ngOnInit() { - this.enableAccountSwitching = enableAccountSwitching(); - const availableVaultTimeoutActions = await firstValueFrom( this.vaultTimeoutSettingsService.availableVaultTimeoutActions$(), ); diff --git a/apps/browser/src/auth/popup/account-switching/services/account-switcher.service.spec.ts b/apps/browser/src/auth/popup/account-switching/services/account-switcher.service.spec.ts index f3be535f00e..13f4a8635df 100644 --- a/apps/browser/src/auth/popup/account-switching/services/account-switcher.service.spec.ts +++ b/apps/browser/src/auth/popup/account-switching/services/account-switcher.service.spec.ts @@ -9,6 +9,7 @@ import { import { AuthService } from "@bitwarden/common/auth/abstractions/auth.service"; import { AvatarService } from "@bitwarden/common/auth/abstractions/avatar.service"; import { AuthenticationStatus } from "@bitwarden/common/auth/enums/authentication-status"; +import { ConfigService } from "@bitwarden/common/platform/abstractions/config/config.service"; import { Environment, EnvironmentService, @@ -37,6 +38,7 @@ describe("AccountSwitcherService", () => { const environmentService = mock(); const logService = mock(); const authService = mock(); + const configService = mock(); let accountSwitcherService: AccountSwitcherService; @@ -60,6 +62,7 @@ describe("AccountSwitcherService", () => { messagingService, environmentService, logService, + configService, authService, ); }); diff --git a/apps/browser/src/auth/popup/account-switching/services/account-switcher.service.ts b/apps/browser/src/auth/popup/account-switching/services/account-switcher.service.ts index 99d2c83283e..0f25ea91c99 100644 --- a/apps/browser/src/auth/popup/account-switching/services/account-switcher.service.ts +++ b/apps/browser/src/auth/popup/account-switching/services/account-switcher.service.ts @@ -7,6 +7,7 @@ import { filter, firstValueFrom, map, + of, switchMap, throwError, timeout, @@ -17,11 +18,14 @@ import { AccountService } from "@bitwarden/common/auth/abstractions/account.serv import { AuthService } from "@bitwarden/common/auth/abstractions/auth.service"; import { AvatarService } from "@bitwarden/common/auth/abstractions/avatar.service"; import { AuthenticationStatus } from "@bitwarden/common/auth/enums/authentication-status"; +import { FeatureFlag } from "@bitwarden/common/enums/feature-flag.enum"; +import { ConfigService } from "@bitwarden/common/platform/abstractions/config/config.service"; import { EnvironmentService } from "@bitwarden/common/platform/abstractions/environment.service"; import { LogService } from "@bitwarden/common/platform/abstractions/log.service"; import { MessagingService } from "@bitwarden/common/platform/abstractions/messaging.service"; import { UserId } from "@bitwarden/common/types/guid"; +import { BrowserApi } from "../../../../platform/browser/browser-api"; import { fromChromeEvent } from "../../../../platform/browser/from-chrome-event"; export type AvailableAccount = { @@ -52,6 +56,7 @@ export class AccountSwitcherService { private messagingService: MessagingService, private environmentService: EnvironmentService, private logService: LogService, + private configService: ConfigService, authService: AuthService, ) { this.availableAccounts$ = combineLatest([ @@ -123,6 +128,19 @@ export class AccountSwitcherService { ); } + /* + * PM-5594: This was a compile-time flag (default true) which made an exception for Safari in platform/flags. + * The truthiness of AccountSwitching has been enshrined at this point, so those compile-time flags have been removed + * in favor of this method to allow easier access to the config service for controlling Safari. Unwinding the Safari + * flag should be more straightforward from this consolidation. + */ + accountSwitchingEnabled$(): Observable { + if (BrowserApi.isSafariApi) { + return this.configService.getFeatureFlag$(FeatureFlag.SafariAccountSwitching); + } + return of(true); + } + get specialAccountAddId() { return this.SPECIAL_ADD_ACCOUNT_ID; } diff --git a/apps/browser/src/autofill/popup/settings/excluded-domains.component.html b/apps/browser/src/autofill/popup/settings/excluded-domains.component.html index 75be3bcc1a0..30170820a27 100644 --- a/apps/browser/src/autofill/popup/settings/excluded-domains.component.html +++ b/apps/browser/src/autofill/popup/settings/excluded-domains.component.html @@ -8,7 +8,9 @@

{{ - accountSwitcherEnabled ? ("excludedDomainsDescAlt" | i18n) : ("excludedDomainsDesc" | i18n) + (accountSwitcherEnabled$ | async) + ? ("excludedDomainsDescAlt" | i18n) + : ("excludedDomainsDesc" | i18n) }}

diff --git a/apps/browser/src/autofill/popup/settings/excluded-domains.component.ts b/apps/browser/src/autofill/popup/settings/excluded-domains.component.ts index e67c826cac6..6714f749d2d 100644 --- a/apps/browser/src/autofill/popup/settings/excluded-domains.component.ts +++ b/apps/browser/src/autofill/popup/settings/excluded-domains.component.ts @@ -15,7 +15,7 @@ import { FormArray, } from "@angular/forms"; import { RouterModule } from "@angular/router"; -import { Subject, takeUntil } from "rxjs"; +import { Observable, Subject, takeUntil } from "rxjs"; import { JslibModule } from "@bitwarden/angular/jslib.module"; import { DomainSettingsService } from "@bitwarden/common/autofill/services/domain-settings.service"; @@ -35,7 +35,7 @@ import { TypographyModule, } from "@bitwarden/components"; -import { enableAccountSwitching } from "../../../platform/flags"; +import { AccountSwitcherService } from "../../../auth/popup/account-switching/services/account-switcher.service"; import { PopOutComponent } from "../../../platform/popup/components/pop-out.component"; import { PopupFooterComponent } from "../../../platform/popup/layout/popup-footer.component"; import { PopupHeaderComponent } from "../../../platform/popup/layout/popup-header.component"; @@ -74,7 +74,8 @@ export class ExcludedDomainsComponent implements AfterViewInit, OnDestroy { @ViewChildren("uriInput") uriInputElements: QueryList> = new QueryList(); - accountSwitcherEnabled = false; + readonly accountSwitcherEnabled$: Observable = + this.accountSwitcherService.accountSwitchingEnabled$(); dataIsPristine = true; isLoading = false; excludedDomainsState: string[] = []; @@ -95,9 +96,8 @@ export class ExcludedDomainsComponent implements AfterViewInit, OnDestroy { private toastService: ToastService, private formBuilder: FormBuilder, private popupRouterCacheService: PopupRouterCacheService, - ) { - this.accountSwitcherEnabled = enableAccountSwitching(); - } + private accountSwitcherService: AccountSwitcherService, + ) {} get domainForms() { return this.domainListForm.get("domains") as FormArray; diff --git a/apps/browser/src/platform/flags.ts b/apps/browser/src/platform/flags.ts index 2b1040bcd8a..30441d42979 100644 --- a/apps/browser/src/platform/flags.ts +++ b/apps/browser/src/platform/flags.ts @@ -8,12 +8,8 @@ import { import { GroupPolicyEnvironment } from "../admin-console/types/group-policy-environment"; -import { BrowserApi } from "./browser/browser-api"; - // required to avoid linting errors when there are no flags -export type Flags = { - accountSwitching?: boolean; -} & SharedFlags; +export type Flags = SharedFlags; // required to avoid linting errors when there are no flags export type DevFlags = { @@ -31,14 +27,3 @@ export function devFlagEnabled(flag: keyof DevFlags) { export function devFlagValue(flag: keyof DevFlags) { return baseDevFlagValue(flag); } - -/** Helper method to sync flag specifically for account switching, which as platform-based values. - * If this pattern needs to be repeated, it's better handled by increasing complexity of webpack configurations - * Not by expanding these flag getters. - */ -export function enableAccountSwitching(): boolean { - if (BrowserApi.isSafariApi) { - return false; - } - return flagEnabled("accountSwitching"); -} diff --git a/libs/common/src/enums/feature-flag.enum.ts b/libs/common/src/enums/feature-flag.enum.ts index 0086524a47f..819ae8bd8e2 100644 --- a/libs/common/src/enums/feature-flag.enum.ts +++ b/libs/common/src/enums/feature-flag.enum.ts @@ -18,6 +18,7 @@ export enum FeatureFlag { /* Auth */ PM23801_PrefetchPasswordPrelogin = "pm-23801-prefetch-password-prelogin", + SafariAccountSwitching = "pm-5594-safari-account-switching", /* Autofill */ MacOsNativeCredentialSync = "macos-native-credential-sync", @@ -134,6 +135,7 @@ export const DefaultFeatureFlagValue = { /* Auth */ [FeatureFlag.PM23801_PrefetchPasswordPrelogin]: FALSE, + [FeatureFlag.SafariAccountSwitching]: FALSE, /* Billing */ [FeatureFlag.TrialPaymentOptional]: FALSE,