[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 <acoroiu@bitwarden.com>

* Fix mock function signature

---------

Co-authored-by: Andreas Coroiu <andreas.coroiu@gmail.com>
Co-authored-by: Andreas Coroiu <acoroiu@bitwarden.com>

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() {

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<AccountSettingsSettings>;
+
+  @sessionSync({ initializer: (b: boolean) => b })
+  protected _disableFavicon: BehaviorSubject<boolean>;
 }

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.settingsService.setDisableFavicon(!this.form.value.enableFavicons);
-    await this.stateService.setDisableFavicon(!this.form.value.enableFavicons, {
-      storageLocation: StorageLocation.Disk,
-    });
     this.messagingService.send("refreshCiphers");
   }
 

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) {

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<StateService>,
         },
+        {
+          provide: SettingsService,
+          useValue: {
+            disableFavicon$: new BehaviorSubject(false).asObservable(),
+            getDisableFavicon() {
+              return false;
+            },
+          } as Partial<SettingsService>,
+        },
         {
           provide: AvatarUpdateService,
           useValue: {

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) {

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();
         }

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();
         }

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;
     }

libs/angular/src/vault/components/icon.component.html

@@ -1,11 +1,16 @@
 <div class="icon" aria-hidden="true">
+  <ng-container *ngIf="data$ | async as data">
     <img
-    [src]="image"
+      [src]="data.image"
-    appFallbackSrc="{{ fallbackImage }}"
+      [appFallbackSrc]="data.fallbackImage"
-    *ngIf="imageEnabled && image"
+      *ngIf="data.imageEnabled && data.image"
       alt=""
       decoding="async"
       loading="lazy"
     />
-  <i class="tw-text-muted bwi bwi-lg {{ icon }}" *ngIf="!imageEnabled || !image"></i>
+    <i
+      class="tw-text-muted bwi bwi-lg {{ data.icon }}"
+      *ngIf="!data.imageEnabled || !data.image"
+    ></i>
+  </ng-container>
 </div>

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<string, string> = {
 @Component({
   selector: "app-vault-icon",
   templateUrl: "icon.component.html",
+  changeDetection: ChangeDetectionStrategy.OnPush,
 })
-export class IconComponent implements OnChanges {
+export class IconComponent implements OnInit {
-  @Input() cipher: CipherView;
+  @Input()
-  icon: string;
+  set cipher(value: CipherView) {
-  image: string;
+    this.cipher$.next(value);
-  fallbackImage: string;
+  }
+
+  protected data$: Observable<{
     imageEnabled: boolean;
+    image?: string;
+    fallbackImage: string;
+    icon?: string;
+  }>;
 
-  private iconsUrl: string;
+  private cipher$ = new BehaviorSubject<CipherView>(undefined);
 
-  constructor(environmentService: EnvironmentService, private stateService: StateService) {
+  constructor(
-    this.iconsUrl = environmentService.getIconsUrl();
+    private environmentService: EnvironmentService,
-  }
+    private settingsService: SettingsService
+  ) {}
 
-  async ngOnChanges() {
+  async ngOnInit() {
-    // Components may be re-used when using cdk-virtual-scroll. Which puts the component in a weird state,
+    const iconsUrl = this.environmentService.getIconsUrl();
-    // to avoid this we reset all state variables.
-    this.image = null;
-    this.fallbackImage = null;
-    this.imageEnabled = !(await this.stateService.getDisableFavicon());
-    this.load();
-  }
 
-  protected load() {
+    this.data$ = combineLatest([
-    switch (this.cipher.type) {
+      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:
-        this.icon = "bwi-globe";
+            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 setLoginIcon() {
+            if (cipher.login.uri) {
-    if (this.cipher.login.uri) {
+              let hostnameUri = cipher.login.uri;
-      let hostnameUri = this.cipher.login.uri;
               let isWebsite = false;
 
               if (hostnameUri.indexOf("androidapp://") === 0) {
-        this.icon = "bwi-android";
+                icon = "bwi-android";
-        this.image = null;
+                image = null;
               } else if (hostnameUri.indexOf("iosapp://") === 0) {
-        this.icon = "bwi-apple";
+                icon = "bwi-apple";
-        this.image = null;
+                image = null;
               } else if (
-        this.imageEnabled &&
+                imageEnabled &&
                 hostnameUri.indexOf("://") === -1 &&
                 hostnameUri.indexOf(".") > -1
               ) {
                 hostnameUri = "http://" + hostnameUri;
                 isWebsite = true;
-      } else if (this.imageEnabled) {
+              } else if (imageEnabled) {
                 isWebsite = hostnameUri.indexOf("http") === 0 && hostnameUri.indexOf(".") > -1;
               }
 
-      if (this.imageEnabled && isWebsite) {
+              if (imageEnabled && isWebsite) {
                 try {
-          this.image = this.iconsUrl + "/" + Utils.getHostname(hostnameUri) + "/icon.png";
+                  image = iconsUrl + "/" + Utils.getHostname(hostnameUri) + "/icon.png";
-          this.fallbackImage = "images/bwi-globe.png";
+                  fallbackImage = "images/bwi-globe.png";
                 } catch (e) {
                   // Ignore error since the fallback icon will be shown if image is null.
                 }
               }
             } else {
-      this.image = null;
+              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;
         }
 
-  private setCardIcon() {
+        return {
-    const brand = this.cipher.card.brand;
+          imageEnabled,
-    if (this.imageEnabled && brand in cardIcons) {
+          image,
-      this.icon = "credit-card-icon " + cardIcons[brand];
+          fallbackImage,
-    }
+          icon,
+        };
+      })
+    );
   }
 }

libs/common/src/abstractions/settings.service.ts

@@ -4,8 +4,11 @@ import { AccountSettingsSettings } from "../models/domain/account";
 
 export abstract class SettingsService {
   settings$: Observable<AccountSettingsSettings>;
+  disableFavicon$: Observable<boolean>;
 
   setEquivalentDomains: (equivalentDomains: string[][]) => Promise<any>;
   getEquivalentDomains: (url: string) => Set<string>;
+  setDisableFavicon: (value: boolean) => Promise<any>;
+  getDisableFavicon: () => boolean;
   clear: (userId?: string) => Promise<void>;
 }

libs/common/src/abstractions/state.service.ts

@@ -145,7 +145,13 @@ export abstract class StateService<T extends Account = Account> {
   ) => Promise<void>;
   getDisableContextMenuItem: (options?: StorageOptions) => Promise<boolean>;
   setDisableContextMenuItem: (value: boolean, options?: StorageOptions) => Promise<void>;
+  /**
+   * @deprecated Do not call this, use SettingsService
+   */
   getDisableFavicon: (options?: StorageOptions) => Promise<boolean>;
+  /**
+   * @deprecated Do not call this, use SettingsService
+   */
   setDisableFavicon: (value: boolean, options?: StorageOptions) => Promise<void>;
   getDisableGa: (options?: StorageOptions) => Promise<boolean>;
   setDisableGa: (value: boolean, options?: StorageOptions) => Promise<void>;

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<AccountSettingsSettings> = new BehaviorSubject({});
+  protected _disableFavicon = new BehaviorSubject<boolean>(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<void> {
     if (userId == null || userId == (await this.stateService.getUserId())) {
       this._settings.next({});

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;
   }