-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[PM-25084] Vault Skeleton loading #17321
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weโll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
121489c
1b8393e
bb03eec
ba570a1
2bdd11a
11547fa
ae50b16
ba534af
3de655a
4fdc57c
5b0421b
c2ecaca
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| <!-- tw-p-3 matches the padding of the popup-page --> | ||
| <div | ||
| class="tw-absolute tw-left-0 tw-top-0 tw-size-full tw-p-3 tw-overflow-hidden tw-bg-background-alt" | ||
| > | ||
| <ng-content></ng-content> | ||
| </div> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| import { animate, style, transition, trigger } from "@angular/animations"; | ||
| import { ChangeDetectionStrategy, Component, HostBinding } from "@angular/core"; | ||
|
|
||
| @Component({ | ||
| selector: "vault-fade-in-out-skeleton", | ||
| templateUrl: "./vault-fade-in-out-skeleton.component.html", | ||
| animations: [ | ||
| trigger("fadeInOut", [ | ||
| transition(":enter", [ | ||
| style({ opacity: 0 }), | ||
| animate("100ms ease-in", style({ opacity: 1 })), | ||
| ]), | ||
| transition(":leave", [animate("300ms ease-out", style({ opacity: 0 }))]), | ||
| ]), | ||
| ], | ||
| changeDetection: ChangeDetectionStrategy.OnPush, | ||
| }) | ||
| export class VaultFadeInOutSkeletonComponent { | ||
| @HostBinding("@fadeInOut") fadeInOut = true; | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,18 +1,20 @@ | ||||||||||
| import { LiveAnnouncer } from "@angular/cdk/a11y"; | ||||||||||
| import { CdkVirtualScrollableElement, ScrollingModule } from "@angular/cdk/scrolling"; | ||||||||||
| import { CommonModule } from "@angular/common"; | ||||||||||
| import { AfterViewInit, Component, DestroyRef, OnDestroy, OnInit, ViewChild } from "@angular/core"; | ||||||||||
| import { takeUntilDestroyed } from "@angular/core/rxjs-interop"; | ||||||||||
| import { Router, RouterModule } from "@angular/router"; | ||||||||||
| import { | ||||||||||
| combineLatest, | ||||||||||
| distinctUntilChanged, | ||||||||||
| filter, | ||||||||||
| firstValueFrom, | ||||||||||
| map, | ||||||||||
| Observable, | ||||||||||
| shareReplay, | ||||||||||
| startWith, | ||||||||||
| switchMap, | ||||||||||
| take, | ||||||||||
| tap, | ||||||||||
| } from "rxjs"; | ||||||||||
|
|
||||||||||
| import { JslibModule } from "@bitwarden/angular/jslib.module"; | ||||||||||
|
|
@@ -22,6 +24,8 @@ import { DeactivatedOrg, NoResults, VaultOpen } from "@bitwarden/assets/svg"; | |||||||||
| import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; | ||||||||||
| import { getUserId } from "@bitwarden/common/auth/services/account.service"; | ||||||||||
| import { FeatureFlag } from "@bitwarden/common/enums/feature-flag.enum"; | ||||||||||
| import { ConfigService } from "@bitwarden/common/platform/abstractions/config/config.service"; | ||||||||||
| import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; | ||||||||||
| import { CipherId, CollectionId, OrganizationId, UserId } from "@bitwarden/common/types/guid"; | ||||||||||
| import { CipherService } from "@bitwarden/common/vault/abstractions/cipher.service"; | ||||||||||
| import { CipherType } from "@bitwarden/common/vault/enums"; | ||||||||||
|
|
@@ -41,11 +45,13 @@ import { PopOutComponent } from "../../../../platform/popup/components/pop-out.c | |||||||||
| import { PopupHeaderComponent } from "../../../../platform/popup/layout/popup-header.component"; | ||||||||||
| import { PopupPageComponent } from "../../../../platform/popup/layout/popup-page.component"; | ||||||||||
| import { IntroCarouselService } from "../../services/intro-carousel.service"; | ||||||||||
| import { VaultPopupCopyButtonsService } from "../../services/vault-popup-copy-buttons.service"; | ||||||||||
| import { VaultPopupItemsService } from "../../services/vault-popup-items.service"; | ||||||||||
| import { VaultPopupListFiltersService } from "../../services/vault-popup-list-filters.service"; | ||||||||||
| import { VaultPopupLoadingService } from "../../services/vault-popup-loading.service"; | ||||||||||
| import { VaultPopupScrollPositionService } from "../../services/vault-popup-scroll-position.service"; | ||||||||||
| import { AtRiskPasswordCalloutComponent } from "../at-risk-callout/at-risk-password-callout.component"; | ||||||||||
| import { VaultFadeInOutSkeletonComponent } from "../vault-fade-in-out-skeleton/vault-fade-in-out-skeleton.component"; | ||||||||||
| import { VaultLoadingSkeletonComponent } from "../vault-loading-skeleton/vault-loading-skeleton.component"; | ||||||||||
|
|
||||||||||
| import { BlockedInjectionBanner } from "./blocked-injection-banner/blocked-injection-banner.component"; | ||||||||||
| import { | ||||||||||
|
|
@@ -88,6 +94,8 @@ type VaultState = UnionOfValues<typeof VaultState>; | |||||||||
| SpotlightComponent, | ||||||||||
| RouterModule, | ||||||||||
| TypographyModule, | ||||||||||
| VaultLoadingSkeletonComponent, | ||||||||||
| VaultFadeInOutSkeletonComponent, | ||||||||||
| ], | ||||||||||
| }) | ||||||||||
| export class VaultV2Component implements OnInit, AfterViewInit, OnDestroy { | ||||||||||
|
|
@@ -108,19 +116,30 @@ export class VaultV2Component implements OnInit, AfterViewInit, OnDestroy { | |||||||||
| ); | ||||||||||
|
|
||||||||||
| activeUserId: UserId | null = null; | ||||||||||
|
|
||||||||||
| private loading$ = this.vaultPopupLoadingService.loading$.pipe( | ||||||||||
| distinctUntilChanged(), | ||||||||||
| tap((loading) => { | ||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ๐ญ Finding 3: Accessibility announcement frequency consideration The live announcer fires on every loading state change. With skeleton loaders, loading states may transition more frequently than with the spinner-only approach. Consider whether this could create announcement fatigue for screen reader users, particularly if the vault reloads multiple times in a session. Suggestion: Monitor user feedback to determine if announcement throttling or different announcement strategies would improve the experience. |
||||||||||
| const key = loading ? "loadingVault" : "vaultLoaded"; | ||||||||||
| void this.liveAnnouncer.announce(this.i18nService.translate(key), "polite"); | ||||||||||
| }), | ||||||||||
| ); | ||||||||||
| private skeletonFeatureFlag$ = this.configService.getFeatureFlag$( | ||||||||||
| FeatureFlag.VaultLoadingSkeletons, | ||||||||||
| ); | ||||||||||
|
|
||||||||||
| protected favoriteCiphers$ = this.vaultPopupItemsService.favoriteCiphers$; | ||||||||||
| protected remainingCiphers$ = this.vaultPopupItemsService.remainingCiphers$; | ||||||||||
| protected allFilters$ = this.vaultPopupListFiltersService.allFilters$; | ||||||||||
|
|
||||||||||
| protected loading$ = combineLatest([ | ||||||||||
| this.vaultPopupItemsService.loading$, | ||||||||||
| this.allFilters$, | ||||||||||
| // Added as a dependency to avoid flashing the copyActions on slower devices | ||||||||||
| this.vaultCopyButtonsService.showQuickCopyActions$, | ||||||||||
| ]).pipe( | ||||||||||
| map(([itemsLoading, filters]) => itemsLoading || !filters), | ||||||||||
| shareReplay({ bufferSize: 1, refCount: true }), | ||||||||||
| startWith(true), | ||||||||||
| /** When true, show spinner loading state */ | ||||||||||
| protected showSpinnerLoaders$ = combineLatest([this.loading$, this.skeletonFeatureFlag$]).pipe( | ||||||||||
| map(([loading, skeletonsEnabled]) => loading && !skeletonsEnabled), | ||||||||||
| ); | ||||||||||
|
Comment on lines
+135
to
+138
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like an artifact from a merge conflict.
Suggested change
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jaasen-livefront I actually want this one and the one below it. They represent the two loading states based on the feature flag.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @nick-livefront Ah my bad I thought they had the same name! |
||||||||||
|
|
||||||||||
| /** When true, show skeleton loading state */ | ||||||||||
| protected showSkeletonsLoaders$ = combineLatest([this.loading$, this.skeletonFeatureFlag$]).pipe( | ||||||||||
| map(([loading, skeletonsEnabled]) => loading && skeletonsEnabled), | ||||||||||
| ); | ||||||||||
|
|
||||||||||
| protected newItemItemValues$: Observable<NewItemInitialValues> = | ||||||||||
|
|
@@ -150,14 +169,17 @@ export class VaultV2Component implements OnInit, AfterViewInit, OnDestroy { | |||||||||
| private vaultPopupItemsService: VaultPopupItemsService, | ||||||||||
| private vaultPopupListFiltersService: VaultPopupListFiltersService, | ||||||||||
| private vaultScrollPositionService: VaultPopupScrollPositionService, | ||||||||||
| private vaultPopupLoadingService: VaultPopupLoadingService, | ||||||||||
| private accountService: AccountService, | ||||||||||
| private destroyRef: DestroyRef, | ||||||||||
| private cipherService: CipherService, | ||||||||||
| private dialogService: DialogService, | ||||||||||
| private vaultCopyButtonsService: VaultPopupCopyButtonsService, | ||||||||||
| private introCarouselService: IntroCarouselService, | ||||||||||
| private nudgesService: NudgesService, | ||||||||||
| private router: Router, | ||||||||||
| private liveAnnouncer: LiveAnnouncer, | ||||||||||
| private i18nService: I18nService, | ||||||||||
| private configService: ConfigService, | ||||||||||
| ) { | ||||||||||
| combineLatest([ | ||||||||||
| this.vaultPopupItemsService.emptyVault$, | ||||||||||
|
|
||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,72 @@ | ||
| import { TestBed } from "@angular/core/testing"; | ||
| import { firstValueFrom, skip, Subject } from "rxjs"; | ||
|
|
||
| import { VaultPopupCopyButtonsService } from "./vault-popup-copy-buttons.service"; | ||
| import { VaultPopupItemsService } from "./vault-popup-items.service"; | ||
| import { VaultPopupListFiltersService } from "./vault-popup-list-filters.service"; | ||
| import { VaultPopupLoadingService } from "./vault-popup-loading.service"; | ||
|
|
||
| describe("VaultPopupLoadingService", () => { | ||
| let service: VaultPopupLoadingService; | ||
| let itemsLoading$: Subject<boolean>; | ||
| let allFilters$: Subject<any>; | ||
| let showQuickCopyActions$: Subject<boolean>; | ||
|
|
||
| beforeEach(() => { | ||
| itemsLoading$ = new Subject<boolean>(); | ||
| allFilters$ = new Subject<any>(); | ||
| showQuickCopyActions$ = new Subject<boolean>(); | ||
|
|
||
| TestBed.configureTestingModule({ | ||
| providers: [ | ||
| VaultPopupLoadingService, | ||
| { provide: VaultPopupItemsService, useValue: { loading$: itemsLoading$ } }, | ||
| { provide: VaultPopupListFiltersService, useValue: { allFilters$: allFilters$ } }, | ||
| { | ||
| provide: VaultPopupCopyButtonsService, | ||
| useValue: { showQuickCopyActions$: showQuickCopyActions$ }, | ||
| }, | ||
| ], | ||
| }); | ||
|
|
||
| service = TestBed.inject(VaultPopupLoadingService); | ||
| }); | ||
|
|
||
| it("emits true initially", async () => { | ||
| const loading = await firstValueFrom(service.loading$); | ||
|
|
||
| expect(loading).toBe(true); | ||
| }); | ||
|
|
||
| it("emits false when items are loaded and filters are available", async () => { | ||
| const loadingPromise = firstValueFrom(service.loading$.pipe(skip(1))); | ||
|
|
||
| itemsLoading$.next(false); | ||
| allFilters$.next({}); | ||
| showQuickCopyActions$.next(true); | ||
|
|
||
| expect(await loadingPromise).toBe(false); | ||
| }); | ||
|
|
||
| it("emits true when filters are not available", async () => { | ||
| const loadingPromise = firstValueFrom(service.loading$.pipe(skip(2))); | ||
|
|
||
| itemsLoading$.next(false); | ||
| allFilters$.next({}); | ||
| showQuickCopyActions$.next(true); | ||
| allFilters$.next(null); | ||
|
|
||
| expect(await loadingPromise).toBe(true); | ||
| }); | ||
|
|
||
| it("emits true when items are loading", async () => { | ||
| const loadingPromise = firstValueFrom(service.loading$.pipe(skip(2))); | ||
|
|
||
| itemsLoading$.next(false); | ||
| allFilters$.next({}); | ||
| showQuickCopyActions$.next(true); | ||
| itemsLoading$.next(true); | ||
|
|
||
| expect(await loadingPromise).toBe(true); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,27 @@ | ||||||||||||
| import { inject, Injectable } from "@angular/core"; | ||||||||||||
| import { combineLatest, map, shareReplay, startWith } from "rxjs"; | ||||||||||||
|
|
||||||||||||
| import { VaultPopupCopyButtonsService } from "./vault-popup-copy-buttons.service"; | ||||||||||||
| import { VaultPopupItemsService } from "./vault-popup-items.service"; | ||||||||||||
| import { VaultPopupListFiltersService } from "./vault-popup-list-filters.service"; | ||||||||||||
|
|
||||||||||||
| @Injectable({ | ||||||||||||
| providedIn: "root", | ||||||||||||
| }) | ||||||||||||
| export class VaultPopupLoadingService { | ||||||||||||
| private vaultPopupItemsService = inject(VaultPopupItemsService); | ||||||||||||
| private vaultPopupListFiltersService = inject(VaultPopupListFiltersService); | ||||||||||||
| private vaultCopyButtonsService = inject(VaultPopupCopyButtonsService); | ||||||||||||
|
|
||||||||||||
| /** Loading state of the vault */ | ||||||||||||
| loading$ = combineLatest([ | ||||||||||||
| this.vaultPopupItemsService.loading$, | ||||||||||||
| this.vaultPopupListFiltersService.allFilters$, | ||||||||||||
| // Added as a dependency to avoid flashing the copyActions on slower devices | ||||||||||||
| this.vaultCopyButtonsService.showQuickCopyActions$, | ||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ๐จ Finding 2: Unused observable in combineLatest reduces clarity The comment states Suggested improvement: If the intent is to ensure
Suggested change
Or if it's truly just to delay emissions, consider using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ๐จ Finding 2: Unused observable reduces code clarity The Impact: Unnecessary re-renders and reduced code maintainability. Suggested fix: Either use the value in the calculation or use // Option 1: Use the value if it affects loading
this.vaultCopyButtonsService.showQuickCopyActions$,
]).pipe(
map(([itemsLoading, filters, copyActionsReady]) => itemsLoading || !filters || !copyActionsReady),
// Option 2: If just for timing, use withLatestFrom
this.vaultPopupItemsService.loading$.pipe(
withLatestFrom(
this.vaultPopupListFiltersService.allFilters$,
this.vaultCopyButtonsService.showQuickCopyActions$
),
map(([itemsLoading, filters, _]) => itemsLoading || !filters), |
||||||||||||
| ]).pipe( | ||||||||||||
| map(([itemsLoading, filters]) => itemsLoading || !filters), | ||||||||||||
| shareReplay({ bufferSize: 1, refCount: true }), | ||||||||||||
| startWith(true), | ||||||||||||
| ); | ||||||||||||
| } | ||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
โ Finding 1: Component class name mismatch with file name
The class is named
VaultFadeInOutSkeletonComponent(includes "Out") but the file is namedvault-fade-in-skeleton.component.ts(no "Out"). This inconsistency violates naming conventions where the class name should match the file name.Suggested fix:
Then update the selector and any imports accordingly.