From 18f84008c488978331234829986be0bc334cb9fa Mon Sep 17 00:00:00 2001 From: Nick Krantz <125900171+nick-livefront@users.noreply.github.com> Date: Wed, 19 Mar 2025 16:01:19 -0500 Subject: [PATCH] [PM-19032] Live Sync on Desktop (#13851) * migrate the vault-items to an observables rather than async/promises - this helps keep data in sync with the service state and avoids race conditions * migrate the view component to an observables rather than async/promises - this helps keep data in sync with the service state and avoids race conditions --- .../vault/app/vault/vault-items.component.ts | 3 - .../src/vault/app/vault/vault.component.ts | 1 - .../src/vault/app/vault/view.component.ts | 4 +- .../vault/components/vault-items.component.ts | 90 +++++++----- .../src/vault/components/view.component.ts | 135 +++++++++++------- 5 files changed, 135 insertions(+), 98 deletions(-) diff --git a/apps/desktop/src/vault/app/vault/vault-items.component.ts b/apps/desktop/src/vault/app/vault/vault-items.component.ts index 5d7285e570b..172a9d10169 100644 --- a/apps/desktop/src/vault/app/vault/vault-items.component.ts +++ b/apps/desktop/src/vault/app/vault/vault-items.component.ts @@ -28,9 +28,6 @@ export class VaultItemsComponent extends BaseVaultItemsComponent { // eslint-disable-next-line rxjs-angular/prefer-takeuntil searchBarService.searchText$.pipe(distinctUntilChanged()).subscribe((searchText) => { this.searchText = searchText; - // FIXME: Verify that this floating promise is intentional. If it is, add an explanatory comment and ensure there is proper error handling. - // eslint-disable-next-line @typescript-eslint/no-floating-promises - this.search(200); }); } diff --git a/apps/desktop/src/vault/app/vault/vault.component.ts b/apps/desktop/src/vault/app/vault/vault.component.ts index 6f844a7bf51..e28a1626a1c 100644 --- a/apps/desktop/src/vault/app/vault/vault.component.ts +++ b/apps/desktop/src/vault/app/vault/vault.component.ts @@ -497,7 +497,6 @@ export class VaultComponent implements OnInit, OnDestroy { this.action = "view"; await this.vaultItemsComponent.refresh(); await this.cipherService.clearCache(this.activeUserId); - await this.viewComponent.load(); this.go(); } diff --git a/apps/desktop/src/vault/app/vault/view.component.ts b/apps/desktop/src/vault/app/vault/view.component.ts index aee1f34437b..deb7e8090de 100644 --- a/apps/desktop/src/vault/app/vault/view.component.ts +++ b/apps/desktop/src/vault/app/vault/view.component.ts @@ -120,9 +120,7 @@ export class ViewComponent extends BaseViewComponent implements OnInit, OnDestro } async ngOnChanges() { - await super.load(); - - if (this.cipher.decryptionFailure) { + if (this.cipher?.decryptionFailure) { DecryptionFailureDialogComponent.open(this.dialogService, { cipherIds: [this.cipherId as CipherId], }); diff --git a/libs/angular/src/vault/components/vault-items.component.ts b/libs/angular/src/vault/components/vault-items.component.ts index fa25bfc8254..9db6aad1bd3 100644 --- a/libs/angular/src/vault/components/vault-items.component.ts +++ b/libs/angular/src/vault/components/vault-items.component.ts @@ -1,13 +1,13 @@ // FIXME: Update this file to be type safe and remove this and next line // @ts-strict-ignore import { Directive, EventEmitter, Input, OnDestroy, OnInit, Output } from "@angular/core"; -import { BehaviorSubject, Subject, firstValueFrom, from, switchMap, takeUntil } from "rxjs"; +import { takeUntilDestroyed } from "@angular/core/rxjs-interop"; +import { BehaviorSubject, Subject, combineLatest, filter, from, switchMap, takeUntil } from "rxjs"; import { SearchService } from "@bitwarden/common/abstractions/search.service"; import { Organization } from "@bitwarden/common/admin-console/models/domain/organization"; import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; import { getUserId } from "@bitwarden/common/auth/services/account.service"; -import { UserId } from "@bitwarden/common/types/guid"; import { CipherService } from "@bitwarden/common/vault/abstractions/cipher.service"; import { CipherView } from "@bitwarden/common/vault/models/view/cipher.view"; @@ -21,16 +21,17 @@ export class VaultItemsComponent implements OnInit, OnDestroy { loaded = false; ciphers: CipherView[] = []; - filter: (cipher: CipherView) => boolean = null; deleted = false; organization: Organization; protected searchPending = false; + /** Construct filters as an observable so it can be appended to the cipher stream. */ + private _filter$ = new BehaviorSubject<(cipher: CipherView) => boolean | null>(null); private destroy$ = new Subject(); - private searchTimeout: any = null; private isSearchable: boolean = false; private _searchText$ = new BehaviorSubject(""); + get searchText() { return this._searchText$.value; } @@ -38,11 +39,21 @@ export class VaultItemsComponent implements OnInit, OnDestroy { this._searchText$.next(value); } + get filter() { + return this._filter$.value; + } + + set filter(value: (cipher: CipherView) => boolean | null) { + this._filter$.next(value); + } + constructor( protected searchService: SearchService, protected cipherService: CipherService, protected accountService: AccountService, - ) {} + ) { + this.subscribeToCiphers(); + } ngOnInit(): void { this._searchText$ @@ -77,23 +88,6 @@ export class VaultItemsComponent implements OnInit, OnDestroy { async applyFilter(filter: (cipher: CipherView) => boolean = null) { this.filter = filter; - await this.search(null); - } - - async search(timeout: number = null, indexedCiphers?: CipherView[]) { - this.searchPending = false; - if (this.searchTimeout != null) { - clearTimeout(this.searchTimeout); - } - if (timeout == null) { - await this.doSearch(indexedCiphers); - return; - } - this.searchPending = true; - this.searchTimeout = setTimeout(async () => { - await this.doSearch(indexedCiphers); - this.searchPending = false; - }, timeout); } selectCipher(cipher: CipherView) { @@ -118,24 +112,42 @@ export class VaultItemsComponent implements OnInit, OnDestroy { protected deletedFilter: (cipher: CipherView) => boolean = (c) => c.isDeleted === this.deleted; - protected async doSearch(indexedCiphers?: CipherView[], userId?: UserId) { - // Get userId from activeAccount if not provided from parent stream - if (!userId) { - userId = await firstValueFrom(getUserId(this.accountService.activeAccount$)); - } + /** + * Creates stream of dependencies that results in the list of ciphers to display + * within the vault list. + * + * Note: This previously used promises but race conditions with how the ciphers were + * stored in electron. Using observables is more reliable as fresh values will always + * cascade through the components. + */ + private subscribeToCiphers() { + getUserId(this.accountService.activeAccount$) + .pipe( + switchMap((userId) => + combineLatest([ + this.cipherService.cipherViews$(userId).pipe(filter((ciphers) => ciphers != null)), + this.cipherService.failedToDecryptCiphers$(userId), + this._searchText$, + this._filter$, + ]), + ), + switchMap(([indexedCiphers, failedCiphers, searchText, filter]) => { + let allCiphers = indexedCiphers ?? []; + const _failedCiphers = failedCiphers ?? []; - indexedCiphers = - indexedCiphers ?? (await firstValueFrom(this.cipherService.cipherViews$(userId))); + allCiphers = [..._failedCiphers, ...allCiphers]; - const failedCiphers = await firstValueFrom(this.cipherService.failedToDecryptCiphers$(userId)); - if (failedCiphers != null && failedCiphers.length > 0) { - indexedCiphers = [...failedCiphers, ...indexedCiphers]; - } - - this.ciphers = await this.searchService.searchCiphers( - this.searchText, - [this.filter, this.deletedFilter], - indexedCiphers, - ); + return this.searchService.searchCiphers( + searchText, + [filter, this.deletedFilter], + allCiphers, + ); + }), + takeUntilDestroyed(), + ) + .subscribe((ciphers) => { + this.ciphers = ciphers; + this.loaded = true; + }); } } diff --git a/libs/angular/src/vault/components/view.component.ts b/libs/angular/src/vault/components/view.component.ts index a927c32105e..d38e7d7b2d9 100644 --- a/libs/angular/src/vault/components/view.component.ts +++ b/libs/angular/src/vault/components/view.component.ts @@ -11,7 +11,17 @@ import { OnInit, Output, } from "@angular/core"; -import { filter, firstValueFrom, map, Observable } from "rxjs"; +import { + BehaviorSubject, + combineLatest, + filter, + firstValueFrom, + map, + Observable, + of, + switchMap, + tap, +} from "rxjs"; import { ApiService } from "@bitwarden/common/abstractions/api.service"; import { AuditService } from "@bitwarden/common/abstractions/audit.service"; @@ -49,7 +59,18 @@ const BroadcasterSubscriptionId = "BaseViewComponent"; @Directive() export class ViewComponent implements OnDestroy, OnInit { - @Input() cipherId: string; + /** Observable of cipherId$ that will update each time the `Input` updates */ + private _cipherId$ = new BehaviorSubject(null); + + @Input() + set cipherId(value: string) { + this._cipherId$.next(value); + } + + get cipherId(): string { + return this._cipherId$.getValue(); + } + @Input() collectionId: string; @Output() onEditCipher = new EventEmitter(); @Output() onCloneCipher = new EventEmitter(); @@ -125,13 +146,30 @@ export class ViewComponent implements OnDestroy, OnInit { switch (message.command) { case "syncCompleted": if (message.successfully) { - await this.load(); this.changeDetectorRef.detectChanges(); } break; } }); }); + + // Set up the subscription to the activeAccount$ and cipherId$ observables + combineLatest([this.accountService.activeAccount$.pipe(getUserId), this._cipherId$]) + .pipe( + tap(() => this.cleanUp()), + switchMap(([userId, cipherId]) => { + const cipher$ = this.cipherService.cipherViews$(userId).pipe( + map((ciphers) => ciphers?.find((c) => c.id === cipherId)), + filter((cipher) => !!cipher), + ); + return combineLatest([of(userId), cipher$]); + }), + ) + .subscribe(([userId, cipher]) => { + this.cipher = cipher; + + void this.constructCipherDetails(userId); + }); } ngOnDestroy() { @@ -139,55 +177,6 @@ export class ViewComponent implements OnDestroy, OnInit { this.cleanUp(); } - async load() { - this.cleanUp(); - - // Grab individual cipher from `cipherViews$` for the most up-to-date information - const activeUserId = await firstValueFrom(this.accountService.activeAccount$.pipe(getUserId)); - this.cipher = await firstValueFrom( - this.cipherService.cipherViews$(activeUserId).pipe( - map((ciphers) => ciphers?.find((c) => c.id === this.cipherId)), - filter((cipher) => !!cipher), - ), - ); - - this.canAccessPremium = await firstValueFrom( - this.billingAccountProfileStateService.hasPremiumFromAnySource$(activeUserId), - ); - this.showPremiumRequiredTotp = - this.cipher.login.totp && !this.canAccessPremium && !this.cipher.organizationUseTotp; - this.canDeleteCipher$ = this.cipherAuthorizationService.canDeleteCipher$(this.cipher, [ - this.collectionId as CollectionId, - ]); - - if (this.cipher.folderId) { - this.folder = await ( - await firstValueFrom(this.folderService.folderViews$(activeUserId)) - ).find((f) => f.id == this.cipher.folderId); - } - - if ( - this.cipher.type === CipherType.Login && - this.cipher.login.totp && - (this.cipher.organizationUseTotp || this.canAccessPremium) - ) { - await this.totpUpdateCode(); - const interval = this.totpService.getTimeInterval(this.cipher.login.totp); - await this.totpTick(interval); - - this.totpInterval = setInterval(async () => { - await this.totpTick(interval); - }, 1000); - } - - if (this.previousCipherId !== this.cipherId) { - // FIXME: Verify that this floating promise is intentional. If it is, add an explanatory comment and ensure there is proper error handling. - // eslint-disable-next-line @typescript-eslint/no-floating-promises - this.eventCollectionService.collect(EventType.Cipher_ClientViewed, this.cipherId); - } - this.previousCipherId = this.cipherId; - } - async edit() { if (await this.promptPassword()) { this.onEditCipher.emit(this.cipher); @@ -567,4 +556,46 @@ export class ViewComponent implements OnDestroy, OnInit { await this.totpUpdateCode(); } } + + /** + * When a cipher is viewed, construct all details for the view that are not directly + * available from the cipher object itself. + */ + private async constructCipherDetails(userId: UserId) { + this.canAccessPremium = await firstValueFrom( + this.billingAccountProfileStateService.hasPremiumFromAnySource$(userId), + ); + this.showPremiumRequiredTotp = + this.cipher.login.totp && !this.canAccessPremium && !this.cipher.organizationUseTotp; + this.canDeleteCipher$ = this.cipherAuthorizationService.canDeleteCipher$(this.cipher, [ + this.collectionId as CollectionId, + ]); + + if (this.cipher.folderId) { + this.folder = await ( + await firstValueFrom(this.folderService.folderViews$(userId)) + ).find((f) => f.id == this.cipher.folderId); + } + + if ( + this.cipher.type === CipherType.Login && + this.cipher.login.totp && + (this.cipher.organizationUseTotp || this.canAccessPremium) + ) { + await this.totpUpdateCode(); + const interval = this.totpService.getTimeInterval(this.cipher.login.totp); + await this.totpTick(interval); + + this.totpInterval = setInterval(async () => { + await this.totpTick(interval); + }, 1000); + } + + if (this.previousCipherId !== this.cipherId) { + // FIXME: Verify that this floating promise is intentional. If it is, add an explanatory comment and ensure there is proper error handling. + // eslint-disable-next-line @typescript-eslint/no-floating-promises + this.eventCollectionService.collect(EventType.Cipher_ClientViewed, this.cipherId); + } + this.previousCipherId = this.cipherId; + } }