Skip to content

[PM-26687] send skeleton #17333

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.

Sign up for GitHub

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

Merged
merged 11 commits into from
Nov 14, 2025
Merged

[PM-26687] send skeleton #17333

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
b3d5c0a
adding skeleton to send
Jingo88 Nov 10, 2025
604c041
apply sendLoading in dom
Jingo88 Nov 11, 2025
f4dfbd0
show search bar while skeleton is there are send items
Jingo88 Nov 11, 2025
9a6ce54
adding flag check for skeleton in send
Jingo88 Nov 11, 2025
c0d1044
update vault skeleton import in send
Jingo88 Nov 12, 2025
dc84440
empty state for send should show regardless of skeleton loading
Jingo88 Nov 12, 2025
96b4399
Merge branch 'main' into PM-27523-send-skeleton
Jingo88 Nov 12, 2025
26e34fe
adding shareReplay
Jingo88 Nov 13, 2025
90c3ad0
apply skeletonloading observable to dom
Jingo88 Nov 13, 2025
313fd45
remove liveannouncer
Jingo88 Nov 13, 2025
fb400c7
remove unused translation keys
Jingo88 Nov 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions apps/browser/src/_locales/en/messages.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5812,6 +5812,12 @@
"vaultLoaded": {
"message": "Vault loaded"
},
"loadingSendPage": {
"message": "Loading send page"
},
"sendPageLoaded": {
"message": "Send page loaded"
},
"settingDisabledByPolicy": {
"message": "This setting is disabled by your organization's policy.",
"description": "This hint text is displayed when a user setting is disabled due to an organization policy."
Expand Down
11 changes: 8 additions & 3 deletions apps/browser/src/tools/popup/send-v2/send-v2.component.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
<popup-page [loading]="sendsLoading$ | async">
<popup-page [loading]="showSpinnerLoaders$ | async" [hideOverflow]="showSkeletonsLoaders$ | async">
<popup-header slot="header" [pageTitle]="'send' | i18n">
<ng-container slot="end">
<tools-new-send-dropdown *ngIf="!sendsDisabled"></tools-new-send-dropdown>
<app-pop-out></app-pop-out>
<app-current-account></app-current-account>
</ng-container>
</popup-header>
<ng-container slot="above-scroll-area" *ngIf="!(sendsLoading$ | async)">
<ng-container slot="above-scroll-area">
<bit-callout *ngIf="sendsDisabled" [title]="'sendDisabled' | i18n">
{{ "sendDisabledWarning" | i18n }}
</bit-callout>
Expand Down Expand Up @@ -34,7 +34,7 @@
</bit-no-items>
</div>

<ng-container *ngIf="listState !== sendState.Empty">
<ng-container *ngIf="listState !== sendState.Empty && !(sendsLoading$ | async)">
<div
*ngIf="listState === sendState.NoResults"
class="tw-flex tw-flex-col tw-justify-center tw-h-auto tw-pt-12"
Expand All @@ -46,4 +46,9 @@
</div>
<app-send-list-items-container [headerText]="title | i18n" [sends]="sends$ | async" />
</ng-container>
@if (sendsLoading$ | async) {
Copy link

@claude claude bot Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💭 Inconsistent control flow syntax

The template mixes modern control flow syntax (@if on line 49) with legacy structural directives (*ngIf on lines 4, 10, 13, 20, 28, 30, 37, 39).

While both are valid, consistent use of one pattern improves maintainability. Consider whether modernizing all to @if/@for is in scope for this PR or should be addressed separately.

<vault-fade-in-skeleton>
<vault-loading-skeleton></vault-loading-skeleton>
</vault-fade-in-skeleton>
}
</popup-page>
37 changes: 35 additions & 2 deletions apps/browser/src/tools/popup/send-v2/send-v2.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,20 @@
import { LiveAnnouncer } from "@angular/cdk/a11y";
import { CommonModule } from "@angular/common";
import { Component, OnDestroy } from "@angular/core";
import { takeUntilDestroyed } from "@angular/core/rxjs-interop";
import { combineLatest, switchMap } from "rxjs";
import { combineLatest, distinctUntilChanged, map, switchMap, tap } from "rxjs";

import { JslibModule } from "@bitwarden/angular/jslib.module";
import { NoResults, NoSendsIcon } from "@bitwarden/assets/svg";
import { VaultLoadingSkeletonComponent } from "@bitwarden/browser/vault/popup/components/vault-loading-skeleton/vault-loading-skeleton.component";
import { BrowserPremiumUpgradePromptService } from "@bitwarden/browser/vault/popup/services/browser-premium-upgrade-prompt.service";
import { PolicyService } from "@bitwarden/common/admin-console/abstractions/policy/policy.service.abstraction";
import { PolicyType } from "@bitwarden/common/admin-console/enums";
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 { SendType } from "@bitwarden/common/tools/send/enums/send-type";
import { PremiumUpgradePromptService } from "@bitwarden/common/vault/abstractions/premium-upgrade-prompt.service";
import {
Expand All @@ -31,6 +36,7 @@ import { CurrentAccountComponent } from "../../../auth/popup/account-switching/c
import { PopOutComponent } from "../../../platform/popup/components/pop-out.component";
import { PopupHeaderComponent } from "../../../platform/popup/layout/popup-header.component";
import { PopupPageComponent } from "../../../platform/popup/layout/popup-page.component";
import { VaultFadeInOutSkeletonComponent } from "../../../vault/popup/components/vault-fade-in-out-skeleton/vault-fade-in-out-skeleton.component";

// FIXME: update to use a const object instead of a typescript enum
// eslint-disable-next-line @bitwarden/platform/no-enums
Expand Down Expand Up @@ -64,6 +70,8 @@ export enum SendState {
SendListFiltersComponent,
SendSearchComponent,
TypographyModule,
VaultFadeInOutSkeletonComponent,
VaultLoadingSkeletonComponent,
],
})
export class SendV2Component implements OnDestroy {
Expand All @@ -72,7 +80,29 @@ export class SendV2Component implements OnDestroy {

protected listState: SendState | null = null;
protected sends$ = this.sendItemsService.filteredAndSortedSends$;
protected sendsLoading$ = this.sendItemsService.loading$;
private skeletonFeatureFlag$ = this.configService.getFeatureFlag$(
FeatureFlag.VaultLoadingSkeletons,
);
protected sendsLoading$ = this.sendItemsService.loading$.pipe(
Copy link

@claude claude bot Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💭 Consider extracting to a dedicated loading service

The vault implementation uses VaultPopupLoadingService (vault-popup-loading.service.ts:11-27) to centralize loading logic, making it easier to test and maintain.

Consider creating a SendPopupLoadingService that:

  • Combines sendItemsService.loading$ with any future dependencies
  • Includes startWith(true) for initial state
  • Can be easily mocked in tests

This would improve consistency across the codebase and make the component logic cleaner.

Copy link

@claude claude bot Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Missing startWith(true) for initial skeleton display

The observable doesn't include startWith(true), which means the skeleton won't display on initial component load before the first emission from sendItemsService.loading$.

Recommended addition:

protected sendsLoading$ = this.sendItemsService.loading$.pipe(
  startWith(true), // Add this
  distinctUntilChanged(),
  tap((loading) => {
    const key = loading ? "loadingSendPage" : "sendPageLoaded";
    void this.liveAnnouncer.announce(this.i18nService.translate(key), "polite");
  }),
  shareReplay({ bufferSize: 1, refCount: true }),
);

Reference: vault-popup-loading.service.ts:25 uses startWith(true) to ensure skeleton displays immediately on init.

Copy link

@claude claude bot Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎨 Consider extracting to dedicated loading service

The vault implementation uses VaultPopupLoadingService (vault-popup-loading.service.ts:11-27) to centralize loading logic, improving testability and maintainability.

Consider creating a SendPopupLoadingService that:

  • Combines sendItemsService.loading$ with any future dependencies
  • Includes startWith(true) and shareReplay() for proper initialization and sharing
  • Can be easily mocked in tests (current tests don't verify loading behavior - see send-v2.component.spec.ts)

This would improve consistency with the vault implementation and make the component logic cleaner. However, this is an optional enhancement.

distinctUntilChanged(),
tap((loading) => {
const key = loading ? "loadingSendPage" : "sendPageLoaded";
void this.liveAnnouncer.announce(this.i18nService.translate(key), "polite");
}),
);
Copy link

@claude claude bot Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ Missing test coverage for loading state transitions

The codecov report indicates 2 lines of missing coverage in this component. These loading pipeline operations (distinctUntilChanged, tap with liveAnnouncer) should have tests verifying:

  1. Initial loading state (true)
  2. Transition from loading to loaded
  3. LiveAnnouncer calls with correct i18n keys

Consider adding a test file similar to vault-popup-loading.service.spec.ts:35-71 that validates these behaviors.

Comment on lines 84 to 87
Copy link

@claude claude bot Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Multiple subscription issue: This observable is subscribed to 6+ times (4 direct template subscriptions + 2 via combineLatest in showSpinnerLoaders$/showSkeletonsLoaders$), which means the tap() side effect will fire multiple times per state change.

Recommendation: Add shareReplay({ bufferSize: 1, refCount: true }) to the pipe to prevent duplicate announcements:

protected sendsLoading$ = this.sendItemsService.loading$.pipe(
  distinctUntilChanged(),
  tap((loading) => {
    const key = loading ? "loadingSendPage" : "sendPageLoaded";
    void this.liveAnnouncer.announce(this.i18nService.translate(key), "polite");
  }),
  shareReplay({ bufferSize: 1, refCount: true }),
);

Also consider adding startWith(true) like vault-popup-loading.service.ts:25 to ensure skeleton displays immediately on init.


/** Spinner Loading State */
protected showSpinnerLoaders$ = combineLatest([
this.sendsLoading$,
this.skeletonFeatureFlag$,
]).pipe(map(([loading, skeletonsEnabled]) => loading && !skeletonsEnabled));

/** Skeleton Loading State */
protected showSkeletonsLoaders$ = combineLatest([
this.sendsLoading$,
this.skeletonFeatureFlag$,
]).pipe(map(([loading, skeletonsEnabled]) => loading && skeletonsEnabled));

protected title: string = "allSends";
protected noItemIcon = NoSendsIcon;
protected noResultsIcon = NoResults;
Expand All @@ -84,6 +114,9 @@ export class SendV2Component implements OnDestroy {
protected sendListFiltersService: SendListFiltersService,
private policyService: PolicyService,
private accountService: AccountService,
private liveAnnouncer: LiveAnnouncer,
private i18nService: I18nService,
private configService: ConfigService,
) {
combineLatest([
this.sendItemsService.emptyList$,
Expand Down
Loading