From 671a9115bbaa34ca8f91380ac2c771e33c9ddd60 Mon Sep 17 00:00:00 2001 From: Robyn MacCallum Date: Fri, 28 Apr 2023 15:07:26 -0400 Subject: [PATCH] [PM-1400] Update IconComponent to use OnPush ChangeDetection (#5181) * Add disableFavicon$ to stateService * Change IconComponent's ChangeDetectionStrategy and use disableFavicon$ observable * Only get first result from disableFavicon observable * Move disabledFavicon$ to SettingsService * Update usage of disableFavicon to use SettingsService * Remove getting and setting of disabledFavicon on login * Settings service observable adjustments * Fix for popup initially having a null value for the disableFavicon setting in settingsService * Move disabledFavicon$ subscription to ngOnInit * feat: experiment with observables * Remove SettingsService from browser app component * Fix storybook changes * Update apps/web/src/app/vault/components/vault-items/vault-items.stories.ts Co-authored-by: Andreas Coroiu * Fix mock function signature --------- Co-authored-by: Andreas Coroiu Co-authored-by: Andreas Coroiu --- .../src/popup/settings/options.component.ts | 8 +- .../src/services/browser-settings.service.ts | 6 +- .../src/app/accounts/settings.component.ts | 11 +- .../src/app/settings/preferences.component.ts | 9 +- .../vault-items/vault-items.stories.ts | 10 + .../src/auth/components/lock.component.ts | 2 - .../src/auth/components/login.component.ts | 2 - .../src/auth/components/sso.component.ts | 2 - .../auth/components/two-factor.component.ts | 2 - .../src/vault/components/icon.component.html | 23 ++- .../src/vault/components/icon.component.ts | 174 ++++++++++-------- .../src/abstractions/settings.service.ts | 3 + libs/common/src/abstractions/state.service.ts | 6 + libs/common/src/services/settings.service.ts | 13 ++ libs/common/src/services/state.service.ts | 2 +- 15 files changed, 164 insertions(+), 109 deletions(-) diff --git a/apps/browser/src/popup/settings/options.component.ts b/apps/browser/src/popup/settings/options.component.ts index e3983f7a7d..e5d7f4b80e 100644 --- a/apps/browser/src/popup/settings/options.component.ts +++ b/apps/browser/src/popup/settings/options.component.ts @@ -3,6 +3,7 @@ import { Component, OnInit } from "@angular/core"; import { AbstractThemingService } from "@bitwarden/angular/services/theming/theming.service.abstraction"; import { I18nService } from "@bitwarden/common/abstractions/i18n.service"; import { MessagingService } from "@bitwarden/common/abstractions/messaging.service"; +import { SettingsService } from "@bitwarden/common/abstractions/settings.service"; import { StateService } from "@bitwarden/common/abstractions/state.service"; import { TotpService } from "@bitwarden/common/abstractions/totp.service"; import { ThemeType, UriMatchType } from "@bitwarden/common/enums"; @@ -39,7 +40,8 @@ export class OptionsComponent implements OnInit { private stateService: StateService, private totpService: TotpService, i18nService: I18nService, - private themingService: AbstractThemingService + private themingService: AbstractThemingService, + private settingsService: SettingsService ) { this.themeOptions = [ { name: i18nService.t("default"), value: ThemeType.System }, @@ -89,7 +91,7 @@ export class OptionsComponent implements OnInit { this.enableAutoTotpCopy = !(await this.stateService.getDisableAutoTotpCopy()); - this.enableFavicon = !(await this.stateService.getDisableFavicon()); + this.enableFavicon = !this.settingsService.getDisableFavicon(); this.enableBadgeCounter = !(await this.stateService.getDisableBadgeCounter()); @@ -129,7 +131,7 @@ export class OptionsComponent implements OnInit { } async updateFavicon() { - await this.stateService.setDisableFavicon(!this.enableFavicon); + await this.settingsService.setDisableFavicon(!this.enableFavicon); } async updateBadgeCounter() { diff --git a/apps/browser/src/services/browser-settings.service.ts b/apps/browser/src/services/browser-settings.service.ts index 78b4f76ea6..0fc71eddc6 100644 --- a/apps/browser/src/services/browser-settings.service.ts +++ b/apps/browser/src/services/browser-settings.service.ts @@ -3,9 +3,13 @@ import { BehaviorSubject } from "rxjs"; import { AccountSettingsSettings } from "@bitwarden/common/models/domain/account"; import { SettingsService } from "@bitwarden/common/services/settings.service"; -import { sessionSync } from "../decorators/session-sync-observable"; +import { browserSession, sessionSync } from "../decorators/session-sync-observable"; +@browserSession export class BrowserSettingsService extends SettingsService { @sessionSync({ initializer: (obj: string[][]) => obj }) protected _settings: BehaviorSubject; + + @sessionSync({ initializer: (b: boolean) => b }) + protected _disableFavicon: BehaviorSubject; } diff --git a/apps/desktop/src/app/accounts/settings.component.ts b/apps/desktop/src/app/accounts/settings.component.ts index bf84fc0b21..9ddd357c56 100644 --- a/apps/desktop/src/app/accounts/settings.component.ts +++ b/apps/desktop/src/app/accounts/settings.component.ts @@ -9,10 +9,11 @@ import { CryptoService } from "@bitwarden/common/abstractions/crypto.service"; import { I18nService } from "@bitwarden/common/abstractions/i18n.service"; import { MessagingService } from "@bitwarden/common/abstractions/messaging.service"; import { PlatformUtilsService } from "@bitwarden/common/abstractions/platformUtils.service"; +import { SettingsService } from "@bitwarden/common/abstractions/settings.service"; import { VaultTimeoutSettingsService } from "@bitwarden/common/abstractions/vaultTimeout/vaultTimeoutSettings.service"; import { PolicyService } from "@bitwarden/common/admin-console/abstractions/policy/policy.service.abstraction"; import { PolicyType } from "@bitwarden/common/admin-console/enums"; -import { DeviceType, ThemeType, StorageLocation, KeySuffixOptions } from "@bitwarden/common/enums"; +import { DeviceType, ThemeType, KeySuffixOptions } from "@bitwarden/common/enums"; import { VaultTimeoutAction } from "@bitwarden/common/enums/vault-timeout-action.enum"; import { Utils } from "@bitwarden/common/misc/utils"; @@ -107,7 +108,8 @@ export class SettingsComponent implements OnInit { private messagingService: MessagingService, private cryptoService: CryptoService, private modalService: ModalService, - private themingService: AbstractThemingService + private themingService: AbstractThemingService, + private settingsService: SettingsService ) { const isMac = this.platformUtilsService.getDevice() === DeviceType.MacOsDesktop; @@ -437,10 +439,7 @@ export class SettingsComponent implements OnInit { } async saveFavicons() { - await this.stateService.setDisableFavicon(!this.form.value.enableFavicons); - await this.stateService.setDisableFavicon(!this.form.value.enableFavicons, { - storageLocation: StorageLocation.Disk, - }); + await this.settingsService.setDisableFavicon(!this.form.value.enableFavicons); this.messagingService.send("refreshCiphers"); } diff --git a/apps/web/src/app/settings/preferences.component.ts b/apps/web/src/app/settings/preferences.component.ts index 4eb6258af8..e7bd55dd08 100644 --- a/apps/web/src/app/settings/preferences.component.ts +++ b/apps/web/src/app/settings/preferences.component.ts @@ -6,6 +6,7 @@ import { AbstractThemingService } from "@bitwarden/angular/services/theming/them import { I18nService } from "@bitwarden/common/abstractions/i18n.service"; import { MessagingService } from "@bitwarden/common/abstractions/messaging.service"; import { PlatformUtilsService } from "@bitwarden/common/abstractions/platformUtils.service"; +import { SettingsService } from "@bitwarden/common/abstractions/settings.service"; import { StateService } from "@bitwarden/common/abstractions/state.service"; import { VaultTimeoutSettingsService } from "@bitwarden/common/abstractions/vaultTimeout/vaultTimeoutSettings.service"; import { PolicyService } from "@bitwarden/common/admin-console/abstractions/policy/policy.service.abstraction"; @@ -51,7 +52,8 @@ export class PreferencesComponent implements OnInit { private vaultTimeoutSettingsService: VaultTimeoutSettingsService, private platformUtilsService: PlatformUtilsService, private messagingService: MessagingService, - private themingService: AbstractThemingService + private themingService: AbstractThemingService, + private settingsService: SettingsService ) { this.vaultTimeoutOptions = [ { name: i18nService.t("oneMinute"), value: 1 }, @@ -128,11 +130,10 @@ export class PreferencesComponent implements OnInit { takeUntil(this.destroy$) ) .subscribe(); - const initialFormValues = { vaultTimeout: await this.vaultTimeoutSettingsService.getVaultTimeout(), vaultTimeoutAction: await this.vaultTimeoutSettingsService.getVaultTimeoutAction(), - enableFavicons: !(await this.stateService.getDisableFavicon()), + enableFavicons: !(await this.settingsService.getDisableFavicon()), enableFullWidth: await this.stateService.getEnableFullWidth(), theme: await this.stateService.getTheme(), locale: (await this.stateService.getLocale()) ?? null, @@ -157,7 +158,7 @@ export class PreferencesComponent implements OnInit { values.vaultTimeout, values.vaultTimeoutAction ); - await this.stateService.setDisableFavicon(!values.enableFavicons); + await this.settingsService.setDisableFavicon(!values.enableFavicons); await this.stateService.setEnableFullWidth(values.enableFullWidth); this.messagingService.send("setFullWidth"); if (values.theme !== this.startingTheme) { diff --git a/apps/web/src/app/vault/components/vault-items/vault-items.stories.ts b/apps/web/src/app/vault/components/vault-items/vault-items.stories.ts index 0eae02b3e1..28464289cf 100644 --- a/apps/web/src/app/vault/components/vault-items/vault-items.stories.ts +++ b/apps/web/src/app/vault/components/vault-items/vault-items.stories.ts @@ -5,6 +5,7 @@ import { BehaviorSubject } from "rxjs"; import { AvatarUpdateService } from "@bitwarden/common/abstractions/account/avatar-update.service"; import { EnvironmentService } from "@bitwarden/common/abstractions/environment.service"; +import { SettingsService } from "@bitwarden/common/abstractions/settings.service"; import { StateService } from "@bitwarden/common/abstractions/state.service"; import { OrganizationUserType } from "@bitwarden/common/admin-console/enums"; import { Organization } from "@bitwarden/common/admin-console/models/domain/organization"; @@ -69,6 +70,15 @@ export default { }, } as Partial, }, + { + provide: SettingsService, + useValue: { + disableFavicon$: new BehaviorSubject(false).asObservable(), + getDisableFavicon() { + return false; + }, + } as Partial, + }, { provide: AvatarUpdateService, useValue: { diff --git a/libs/angular/src/auth/components/lock.component.ts b/libs/angular/src/auth/components/lock.component.ts index 9d1266de64..f81bfecc86 100644 --- a/libs/angular/src/auth/components/lock.component.ts +++ b/libs/angular/src/auth/components/lock.component.ts @@ -260,8 +260,6 @@ export class LockComponent implements OnInit, OnDestroy { private async doContinue(evaluatePasswordAfterUnlock: boolean) { await this.stateService.setEverBeenUnlocked(true); - const disableFavicon = await this.stateService.getDisableFavicon(); - await this.stateService.setDisableFavicon(!!disableFavicon); this.messagingService.send("unlocked"); if (evaluatePasswordAfterUnlock) { diff --git a/libs/angular/src/auth/components/login.component.ts b/libs/angular/src/auth/components/login.component.ts index ad7b6e630b..0d7c594e69 100644 --- a/libs/angular/src/auth/components/login.component.ts +++ b/libs/angular/src/auth/components/login.component.ts @@ -152,8 +152,6 @@ export class LoginComponent extends CaptchaProtectedComponent implements OnInit this.router.navigate([this.forcePasswordResetRoute]); } } else { - const disableFavicon = await this.stateService.getDisableFavicon(); - await this.stateService.setDisableFavicon(!!disableFavicon); if (this.onSuccessfulLogin != null) { this.onSuccessfulLogin(); } diff --git a/libs/angular/src/auth/components/sso.component.ts b/libs/angular/src/auth/components/sso.component.ts index 708a484215..83c6494ace 100644 --- a/libs/angular/src/auth/components/sso.component.ts +++ b/libs/angular/src/auth/components/sso.component.ts @@ -212,8 +212,6 @@ export class SsoComponent { this.router.navigate([this.forcePasswordResetRoute]); } } else { - const disableFavicon = await this.stateService.getDisableFavicon(); - await this.stateService.setDisableFavicon(!!disableFavicon); if (this.onSuccessfulLogin != null) { await this.onSuccessfulLogin(); } diff --git a/libs/angular/src/auth/components/two-factor.component.ts b/libs/angular/src/auth/components/two-factor.component.ts index 6573738edc..255fb8aa2d 100644 --- a/libs/angular/src/auth/components/two-factor.component.ts +++ b/libs/angular/src/auth/components/two-factor.component.ts @@ -197,8 +197,6 @@ export class TwoFactorComponent extends CaptchaProtectedComponent implements OnI this.captchaToken ); const response: AuthResult = await this.formPromise; - const disableFavicon = await this.stateService.getDisableFavicon(); - await this.stateService.setDisableFavicon(!!disableFavicon); if (this.handleCaptchaRequired(response)) { return; } diff --git a/libs/angular/src/vault/components/icon.component.html b/libs/angular/src/vault/components/icon.component.html index f5d237d898..fd91d4095c 100644 --- a/libs/angular/src/vault/components/icon.component.html +++ b/libs/angular/src/vault/components/icon.component.html @@ -1,11 +1,16 @@ diff --git a/libs/angular/src/vault/components/icon.component.ts b/libs/angular/src/vault/components/icon.component.ts index 568df7147a..7fac39a638 100644 --- a/libs/angular/src/vault/components/icon.component.ts +++ b/libs/angular/src/vault/components/icon.component.ts @@ -1,7 +1,15 @@ -import { Component, Input, OnChanges } from "@angular/core"; +import { ChangeDetectionStrategy, Component, Input, OnInit } from "@angular/core"; +import { + BehaviorSubject, + combineLatest, + distinctUntilChanged, + filter, + map, + Observable, +} from "rxjs"; import { EnvironmentService } from "@bitwarden/common/abstractions/environment.service"; -import { StateService } from "@bitwarden/common/abstractions/state.service"; +import { SettingsService } from "@bitwarden/common/abstractions/settings.service"; import { Utils } from "@bitwarden/common/misc/utils"; import { CipherType } from "@bitwarden/common/vault/enums/cipher-type"; import { CipherView } from "@bitwarden/common/vault/models/view/cipher.view"; @@ -25,89 +33,101 @@ const cardIcons: Record = { @Component({ selector: "app-vault-icon", templateUrl: "icon.component.html", + changeDetection: ChangeDetectionStrategy.OnPush, }) -export class IconComponent implements OnChanges { - @Input() cipher: CipherView; - icon: string; - image: string; - fallbackImage: string; - imageEnabled: boolean; - - private iconsUrl: string; - - constructor(environmentService: EnvironmentService, private stateService: StateService) { - this.iconsUrl = environmentService.getIconsUrl(); +export class IconComponent implements OnInit { + @Input() + set cipher(value: CipherView) { + this.cipher$.next(value); } - async ngOnChanges() { - // Components may be re-used when using cdk-virtual-scroll. Which puts the component in a weird state, - // to avoid this we reset all state variables. - this.image = null; - this.fallbackImage = null; - this.imageEnabled = !(await this.stateService.getDisableFavicon()); - this.load(); - } + protected data$: Observable<{ + imageEnabled: boolean; + image?: string; + fallbackImage: string; + icon?: string; + }>; - protected load() { - switch (this.cipher.type) { - case CipherType.Login: - this.icon = "bwi-globe"; - this.setLoginIcon(); - break; - case CipherType.SecureNote: - this.icon = "bwi-sticky-note"; - break; - case CipherType.Card: - this.icon = "bwi-credit-card"; - this.setCardIcon(); - break; - case CipherType.Identity: - this.icon = "bwi-id-card"; - break; - default: - break; - } - } + private cipher$ = new BehaviorSubject(undefined); - private setLoginIcon() { - if (this.cipher.login.uri) { - let hostnameUri = this.cipher.login.uri; - let isWebsite = false; + constructor( + private environmentService: EnvironmentService, + private settingsService: SettingsService + ) {} - if (hostnameUri.indexOf("androidapp://") === 0) { - this.icon = "bwi-android"; - this.image = null; - } else if (hostnameUri.indexOf("iosapp://") === 0) { - this.icon = "bwi-apple"; - this.image = null; - } else if ( - this.imageEnabled && - hostnameUri.indexOf("://") === -1 && - hostnameUri.indexOf(".") > -1 - ) { - hostnameUri = "http://" + hostnameUri; - isWebsite = true; - } else if (this.imageEnabled) { - isWebsite = hostnameUri.indexOf("http") === 0 && hostnameUri.indexOf(".") > -1; - } + async ngOnInit() { + const iconsUrl = this.environmentService.getIconsUrl(); - if (this.imageEnabled && isWebsite) { - try { - this.image = this.iconsUrl + "/" + Utils.getHostname(hostnameUri) + "/icon.png"; - this.fallbackImage = "images/bwi-globe.png"; - } catch (e) { - // Ignore error since the fallback icon will be shown if image is null. + this.data$ = combineLatest([ + this.settingsService.disableFavicon$.pipe(distinctUntilChanged()), + this.cipher$.pipe(filter((c) => c !== undefined)), + ]).pipe( + map(([disableFavicon, cipher]) => { + const imageEnabled = !disableFavicon; + let image = undefined; + let fallbackImage = ""; + let icon = undefined; + + switch (cipher.type) { + case CipherType.Login: + icon = "bwi-globe"; + + if (cipher.login.uri) { + let hostnameUri = cipher.login.uri; + let isWebsite = false; + + if (hostnameUri.indexOf("androidapp://") === 0) { + icon = "bwi-android"; + image = null; + } else if (hostnameUri.indexOf("iosapp://") === 0) { + icon = "bwi-apple"; + image = null; + } else if ( + imageEnabled && + hostnameUri.indexOf("://") === -1 && + hostnameUri.indexOf(".") > -1 + ) { + hostnameUri = "http://" + hostnameUri; + isWebsite = true; + } else if (imageEnabled) { + isWebsite = hostnameUri.indexOf("http") === 0 && hostnameUri.indexOf(".") > -1; + } + + if (imageEnabled && isWebsite) { + try { + image = iconsUrl + "/" + Utils.getHostname(hostnameUri) + "/icon.png"; + fallbackImage = "images/bwi-globe.png"; + } catch (e) { + // Ignore error since the fallback icon will be shown if image is null. + } + } + } else { + image = null; + } + break; + case CipherType.SecureNote: + icon = "bwi-sticky-note"; + break; + case CipherType.Card: + icon = "bwi-credit-card"; + if (imageEnabled && cipher.card.brand in cardIcons) { + icon = "credit-card-icon " + cardIcons[cipher.card.brand]; + } + break; + case CipherType.Identity: + icon = "bwi-id-card"; + break; + default: + break; } - } - } else { - this.image = null; - } - } - private setCardIcon() { - const brand = this.cipher.card.brand; - if (this.imageEnabled && brand in cardIcons) { - this.icon = "credit-card-icon " + cardIcons[brand]; - } + return { + imageEnabled, + image, + fallbackImage, + icon, + }; + }) + ); } } diff --git a/libs/common/src/abstractions/settings.service.ts b/libs/common/src/abstractions/settings.service.ts index 0432bfd434..ade21aaaae 100644 --- a/libs/common/src/abstractions/settings.service.ts +++ b/libs/common/src/abstractions/settings.service.ts @@ -4,8 +4,11 @@ import { AccountSettingsSettings } from "../models/domain/account"; export abstract class SettingsService { settings$: Observable; + disableFavicon$: Observable; setEquivalentDomains: (equivalentDomains: string[][]) => Promise; getEquivalentDomains: (url: string) => Set; + setDisableFavicon: (value: boolean) => Promise; + getDisableFavicon: () => boolean; clear: (userId?: string) => Promise; } diff --git a/libs/common/src/abstractions/state.service.ts b/libs/common/src/abstractions/state.service.ts index 57a09d6120..ac5bb70596 100644 --- a/libs/common/src/abstractions/state.service.ts +++ b/libs/common/src/abstractions/state.service.ts @@ -145,7 +145,13 @@ export abstract class StateService { ) => Promise; getDisableContextMenuItem: (options?: StorageOptions) => Promise; setDisableContextMenuItem: (value: boolean, options?: StorageOptions) => Promise; + /** + * @deprecated Do not call this, use SettingsService + */ getDisableFavicon: (options?: StorageOptions) => Promise; + /** + * @deprecated Do not call this, use SettingsService + */ setDisableFavicon: (value: boolean, options?: StorageOptions) => Promise; getDisableGa: (options?: StorageOptions) => Promise; setDisableGa: (value: boolean, options?: StorageOptions) => Promise; diff --git a/libs/common/src/services/settings.service.ts b/libs/common/src/services/settings.service.ts index 026746cbad..5bd7d9c2fb 100644 --- a/libs/common/src/services/settings.service.ts +++ b/libs/common/src/services/settings.service.ts @@ -7,8 +7,10 @@ import { AccountSettingsSettings } from "../models/domain/account"; export class SettingsService implements SettingsServiceAbstraction { protected _settings: BehaviorSubject = new BehaviorSubject({}); + protected _disableFavicon = new BehaviorSubject(null); settings$ = this._settings.asObservable(); + disableFavicon$ = this._disableFavicon.asObservable(); constructor(private stateService: StateService) { this.stateService.activeAccountUnlocked$ @@ -24,8 +26,10 @@ export class SettingsService implements SettingsServiceAbstraction { } const data = await this.stateService.getSettings(); + const disableFavicon = await this.stateService.getDisableFavicon(); this._settings.next(data); + this._disableFavicon.next(disableFavicon); }) ) .subscribe(); @@ -61,6 +65,15 @@ export class SettingsService implements SettingsServiceAbstraction { return new Set(result); } + async setDisableFavicon(value: boolean) { + this._disableFavicon.next(value); + await this.stateService.setDisableFavicon(value); + } + + getDisableFavicon(): boolean { + return this._disableFavicon.getValue(); + } + async clear(userId?: string): Promise { if (userId == null || userId == (await this.stateService.getUserId())) { this._settings.next({}); diff --git a/libs/common/src/services/state.service.ts b/libs/common/src/services/state.service.ts index 8f2e9b7bd3..4a1592e733 100644 --- a/libs/common/src/services/state.service.ts +++ b/libs/common/src/services/state.service.ts @@ -103,7 +103,6 @@ export class StateService< } else if (userId == null) { this.activeAccountUnlockedSubject.next(false); } - // FIXME: This should be refactored into AuthService or a similar service, // as checking for the existence of the crypto key is a low level // implementation detail. @@ -130,6 +129,7 @@ export class StateService< } }); await this.initAccountState(); + this.hasBeenInited = true; }