From 94438d4138e20a97cc1939082628748f27bb7b67 Mon Sep 17 00:00:00 2001 From: Nick Krantz <125900171+nick-livefront@users.noreply.github.com> Date: Fri, 14 Jun 2024 08:24:50 -0500 Subject: [PATCH] [PM-8208] Fix: Product Navigation flash (#9587) * wait until a sync is complete to render the product switcher content * refactor unneeded observables into their own variable * do not show product switcher button until content is loaded * use `ReplaySubject` to ensure that `syncCompleted$` last value is always used --- .../product-switcher.component.html | 1 + .../product-switcher.component.ts | 9 ++- .../shared/product-switcher.service.spec.ts | 65 +++++++++++++++---- .../shared/product-switcher.service.ts | 58 ++++++++++++----- 4 files changed, 102 insertions(+), 31 deletions(-) diff --git a/apps/web/src/app/layouts/product-switcher/product-switcher.component.html b/apps/web/src/app/layouts/product-switcher/product-switcher.component.html index c9956f05e4..f1942a02c2 100644 --- a/apps/web/src/app/layouts/product-switcher/product-switcher.component.html +++ b/apps/web/src/app/layouts/product-switcher/product-switcher.component.html @@ -4,5 +4,6 @@ [bitMenuTriggerFor]="content?.menu" [buttonType]="buttonType" [attr.aria-label]="'switchProducts' | i18n" + *ngIf="products$ | async" > diff --git a/apps/web/src/app/layouts/product-switcher/product-switcher.component.ts b/apps/web/src/app/layouts/product-switcher/product-switcher.component.ts index eff5f08702..8015516639 100644 --- a/apps/web/src/app/layouts/product-switcher/product-switcher.component.ts +++ b/apps/web/src/app/layouts/product-switcher/product-switcher.component.ts @@ -1,6 +1,8 @@ import { AfterViewInit, ChangeDetectorRef, Component, Input } from "@angular/core"; import { IconButtonType } from "@bitwarden/components/src/icon-button/icon-button.component"; + +import { ProductSwitcherService } from "./shared/product-switcher.service"; @Component({ selector: "product-switcher", templateUrl: "./product-switcher.component.html", @@ -21,5 +23,10 @@ export class ProductSwitcherComponent implements AfterViewInit { this.changeDetector.detectChanges(); } - constructor(private changeDetector: ChangeDetectorRef) {} + constructor( + private changeDetector: ChangeDetectorRef, + private productSwitcherService: ProductSwitcherService, + ) {} + + protected readonly products$ = this.productSwitcherService.products$; } diff --git a/apps/web/src/app/layouts/product-switcher/shared/product-switcher.service.spec.ts b/apps/web/src/app/layouts/product-switcher/shared/product-switcher.service.spec.ts index b98c2495e5..07a41e7c94 100644 --- a/apps/web/src/app/layouts/product-switcher/shared/product-switcher.service.spec.ts +++ b/apps/web/src/app/layouts/product-switcher/shared/product-switcher.service.spec.ts @@ -8,6 +8,7 @@ import { OrganizationService } from "@bitwarden/common/admin-console/abstraction import { ProviderService } from "@bitwarden/common/admin-console/abstractions/provider.service"; import { Organization } from "@bitwarden/common/admin-console/models/domain/organization"; import { Provider } from "@bitwarden/common/admin-console/models/domain/provider"; +import { SyncService } from "@bitwarden/common/platform/sync"; import { ProductSwitcherService } from "./product-switcher.service"; @@ -17,8 +18,19 @@ describe("ProductSwitcherService", () => { let organizationService: MockProxy; let providerService: MockProxy; let activeRouteParams = convertToParamMap({ organizationId: "1234" }); + const getLastSync = jest.fn().mockResolvedValue(new Date("2024-05-14")); + + // The service is dependent on the SyncService, which is behind a `setTimeout` + // Most of the tests don't need to test this aspect so `advanceTimersByTime` + // is used to simulate the completion of the sync + function initiateService() { + service = TestBed.inject(ProductSwitcherService); + jest.advanceTimersByTime(201); + } beforeEach(() => { + jest.useFakeTimers(); + getLastSync.mockResolvedValue(new Date("2024-05-14")); router = mock(); organizationService = mock(); providerService = mock(); @@ -46,14 +58,40 @@ describe("ProductSwitcherService", () => { transform: (key: string) => key, }, }, + { + provide: SyncService, + useValue: { getLastSync }, + }, ], }); }); + afterEach(() => { + jest.useRealTimers(); + }); + + describe("SyncService", () => { + it("waits until sync is complete before emitting products", (done) => { + getLastSync.mockResolvedValue(null); + + initiateService(); + + // The subscription will only emit once the sync returns a value + service.products$.subscribe((products) => { + expect(products).toBeDefined(); + done(); + }); + + // Simulate sync completion & advance timers + getLastSync.mockResolvedValue(new Date("2024-05-15")); + jest.advanceTimersByTime(201); + }); + }); + describe("product separation", () => { describe("Password Manager", () => { it("is always included", async () => { - service = TestBed.inject(ProductSwitcherService); + initiateService(); const products = await firstValueFrom(service.products$); @@ -63,7 +101,7 @@ describe("ProductSwitcherService", () => { describe("Secret Manager", () => { it("is included in other when there are no organizations with SM", async () => { - service = TestBed.inject(ProductSwitcherService); + initiateService(); const products = await firstValueFrom(service.products$); @@ -75,7 +113,7 @@ describe("ProductSwitcherService", () => { { id: "1234", canAccessSecretsManager: true, enabled: true }, ] as Organization[]); - service = TestBed.inject(ProductSwitcherService); + initiateService(); const products = await firstValueFrom(service.products$); @@ -85,7 +123,7 @@ describe("ProductSwitcherService", () => { describe("Admin/Organizations", () => { it("includes Organizations in other when there are organizations", async () => { - service = TestBed.inject(ProductSwitcherService); + initiateService(); const products = await firstValueFrom(service.products$); @@ -96,7 +134,7 @@ describe("ProductSwitcherService", () => { it("includes Admin Console in bento when a user has access to it", async () => { organizationService.organizations$ = of([{ id: "1234", isOwner: true }] as Organization[]); - service = TestBed.inject(ProductSwitcherService); + initiateService(); const products = await firstValueFrom(service.products$); @@ -107,8 +145,7 @@ describe("ProductSwitcherService", () => { describe("Provider Portal", () => { it("is not included when there are no providers", async () => { - service = TestBed.inject(ProductSwitcherService); - + initiateService(); const products = await firstValueFrom(service.products$); expect(products.bento.find((p) => p.name === "Provider Portal")).toBeUndefined(); @@ -118,7 +155,7 @@ describe("ProductSwitcherService", () => { it("is included when there are providers", async () => { providerService.getAll.mockResolvedValue([{ id: "67899" }] as Provider[]); - service = TestBed.inject(ProductSwitcherService); + initiateService(); const products = await firstValueFrom(service.products$); @@ -129,7 +166,7 @@ describe("ProductSwitcherService", () => { describe("active product", () => { it("marks Password Manager as active", async () => { - service = TestBed.inject(ProductSwitcherService); + initiateService(); const products = await firstValueFrom(service.products$); @@ -141,7 +178,7 @@ describe("ProductSwitcherService", () => { it("marks Secret Manager as active", async () => { router.url = "/sm/"; - service = TestBed.inject(ProductSwitcherService); + initiateService(); const products = await firstValueFrom(service.products$); @@ -155,7 +192,7 @@ describe("ProductSwitcherService", () => { activeRouteParams = convertToParamMap({ organizationId: "1" }); router.url = "/organizations/"; - service = TestBed.inject(ProductSwitcherService); + initiateService(); const products = await firstValueFrom(service.products$); @@ -168,7 +205,7 @@ describe("ProductSwitcherService", () => { providerService.getAll.mockResolvedValue([{ id: "67899" }] as Provider[]); router.url = "/providers/"; - service = TestBed.inject(ProductSwitcherService); + initiateService(); const products = await firstValueFrom(service.products$); @@ -187,7 +224,7 @@ describe("ProductSwitcherService", () => { { id: "4243", canAccessSecretsManager: true, enabled: true, name: "Org 32" }, ] as Organization[]); - service = TestBed.inject(ProductSwitcherService); + initiateService(); const products = await firstValueFrom(service.products$); @@ -205,7 +242,7 @@ describe("ProductSwitcherService", () => { { id: "4243", isOwner: true, name: "My Org" }, ] as Organization[]); - service = TestBed.inject(ProductSwitcherService); + initiateService(); const products = await firstValueFrom(service.products$); diff --git a/apps/web/src/app/layouts/product-switcher/shared/product-switcher.service.ts b/apps/web/src/app/layouts/product-switcher/shared/product-switcher.service.ts index 1a418ef3b0..434391cd50 100644 --- a/apps/web/src/app/layouts/product-switcher/shared/product-switcher.service.ts +++ b/apps/web/src/app/layouts/product-switcher/shared/product-switcher.service.ts @@ -1,13 +1,6 @@ import { Injectable } from "@angular/core"; -import { - ActivatedRoute, - Event, - NavigationEnd, - NavigationStart, - ParamMap, - Router, -} from "@angular/router"; -import { combineLatest, concatMap, filter, map, Observable, startWith } from "rxjs"; +import { ActivatedRoute, NavigationEnd, NavigationStart, ParamMap, Router } from "@angular/router"; +import { combineLatest, concatMap, filter, map, Observable, ReplaySubject, startWith } from "rxjs"; import { I18nPipe } from "@bitwarden/angular/platform/pipes/i18n.pipe"; import { @@ -16,6 +9,7 @@ import { } from "@bitwarden/common/admin-console/abstractions/organization/organization.service.abstraction"; import { ProviderService } from "@bitwarden/common/admin-console/abstractions/provider.service"; import { Organization } from "@bitwarden/common/admin-console/models/domain/organization"; +import { SyncService } from "@bitwarden/common/platform/sync"; export type ProductSwitcherItem = { /** @@ -59,13 +53,38 @@ export type ProductSwitcherItem = { providedIn: "root", }) export class ProductSwitcherService { + /** + * Emits when the sync service has completed a sync + * + * Without waiting for a sync to be complete, in accurate product information + * can be displayed to the user for a brief moment until the sync is complete + * and all data is available. + */ + private syncCompleted$ = new ReplaySubject(1); + + /** + * Certain events should trigger an update to the `products$` observable but the values + * themselves are not needed. This observable is used to only trigger the update. + */ + private triggerProductUpdate$: Observable = combineLatest([ + this.syncCompleted$, + this.router.events.pipe( + // Product paths need to be updated when routes change, but the router event isn't actually needed + startWith(null), // Start with a null event to trigger the initial combineLatest + filter((e) => e instanceof NavigationEnd || e instanceof NavigationStart || e === null), + ), + ]).pipe(map(() => null)); + constructor( private organizationService: OrganizationService, private providerService: ProviderService, private route: ActivatedRoute, private router: Router, private i18n: I18nPipe, - ) {} + private syncService: SyncService, + ) { + this.pollUntilSynced(); + } products$: Observable<{ bento: ProductSwitcherItem[]; @@ -73,13 +92,9 @@ export class ProductSwitcherService { }> = combineLatest([ this.organizationService.organizations$, this.route.paramMap, - this.router.events.pipe( - // Product paths need to be updated when routes change, but the router event isn't actually needed - startWith(null), // Start with a null event to trigger the initial combineLatest - filter((e) => e instanceof NavigationEnd || e instanceof NavigationStart || e === null), - ), + this.triggerProductUpdate$, ]).pipe( - map(([orgs, ...rest]): [Organization[], ParamMap, Event | null] => { + map(([orgs, ...rest]): [Organization[], ParamMap, void] => { return [ // Sort orgs by name to match the order within the sidebar orgs.sort((a, b) => a.name.localeCompare(b.name)), @@ -186,4 +201,15 @@ export class ProductSwitcherService { }; }), ); + + /** Poll the `syncService` until a sync is completed */ + private pollUntilSynced() { + const interval = setInterval(async () => { + const lastSync = await this.syncService.getLastSync(); + if (lastSync !== null) { + clearInterval(interval); + this.syncCompleted$.next(); + } + }, 200); + } }