From 23cb3e092c778d5a21c005a0e46665e7e75c0088 Mon Sep 17 00:00:00 2001 From: Leslie Tilton <23057410+Banrion@users.noreply.github.com> Date: Fri, 31 Oct 2025 13:59:11 -0500 Subject: [PATCH] [PM-27694] Handle empty report response (#17162) * Consolidate loading state and handle null report from api response * Fix jumping of page when ciphers are still loading * Fix type errors * Fix loading state --- .../risk-insights/models/report-models.ts | 12 +- .../services/api/risk-insights-api.service.ts | 4 +- ...risk-insights-orchestrator.service.spec.ts | 35 +--- .../risk-insights-orchestrator.service.ts | 178 ++++++++---------- .../domain/risk-insights-report.service.ts | 6 +- .../view/risk-insights-data.service.ts | 28 ++- .../activity/all-activity.component.html | 2 +- .../activity/all-activity.component.ts | 3 + .../all-applications.component.html | 2 +- .../all-applications.component.ts | 6 +- .../critical-applications.component.html | 10 +- .../critical-applications.component.ts | 1 - .../risk-insights.component.html | 171 +++++++++-------- .../risk-insights.component.ts | 82 ++------ 14 files changed, 237 insertions(+), 303 deletions(-) diff --git a/bitwarden_license/bit-common/src/dirt/reports/risk-insights/models/report-models.ts b/bitwarden_license/bit-common/src/dirt/reports/risk-insights/models/report-models.ts index cada0acb69..eecd8256c7 100644 --- a/bitwarden_license/bit-common/src/dirt/reports/risk-insights/models/report-models.ts +++ b/bitwarden_license/bit-common/src/dirt/reports/risk-insights/models/report-models.ts @@ -98,6 +98,15 @@ export type ReportResult = CipherView & { scoreKey: number; }; +export const ReportStatus = Object.freeze({ + Initializing: 1, + Loading: 2, + Complete: 3, + Error: 4, +} as const); + +export type ReportStatus = (typeof ReportStatus)[keyof typeof ReportStatus]; + export interface RiskInsightsData { id: OrganizationReportId; creationDate: Date; @@ -108,10 +117,9 @@ export interface RiskInsightsData { } export interface ReportState { - loading: boolean; + status: ReportStatus; error: string | null; data: RiskInsightsData | null; - organizationId?: string; } // TODO Make Versioned models for structure changes diff --git a/bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/api/risk-insights-api.service.ts b/bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/api/risk-insights-api.service.ts index 5b094e8dec..e221d68ca7 100644 --- a/bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/api/risk-insights-api.service.ts +++ b/bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/api/risk-insights-api.service.ts @@ -29,7 +29,9 @@ export class RiskInsightsApiService { true, ); return from(dbResponse).pipe( - map((response) => new GetRiskInsightsReportResponse(response)), + // As of this change, the server doesn't return a 404 if a report is not found + // Handle null response if server returns nothing + map((response) => (response ? new GetRiskInsightsReportResponse(response) : null)), catchError((error: unknown) => { if (error instanceof ErrorResponse && error.statusCode === 404) { return of(null); // Handle 404 by returning null or an appropriate default value diff --git a/bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/domain/risk-insights-orchestrator.service.spec.ts b/bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/domain/risk-insights-orchestrator.service.spec.ts index 9894c6522a..bc0d4fc789 100644 --- a/bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/domain/risk-insights-orchestrator.service.spec.ts +++ b/bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/domain/risk-insights-orchestrator.service.spec.ts @@ -10,7 +10,7 @@ import { CipherService } from "@bitwarden/common/vault/abstractions/cipher.servi import { LogService } from "@bitwarden/logging"; import { createNewSummaryData } from "../../helpers"; -import { RiskInsightsData, SaveRiskInsightsReportResponse } from "../../models"; +import { ReportStatus, RiskInsightsData, SaveRiskInsightsReportResponse } from "../../models"; import { RiskInsightsMetrics } from "../../models/domain/risk-insights-metrics"; import { mockMemberCipherDetailsResponse } from "../../models/mocks/member-cipher-details-response.mock"; import { @@ -105,34 +105,6 @@ describe("RiskInsightsOrchestratorService", () => { }); describe("fetchReport", () => { - it("should call with correct org and user IDs and emit ReportState", (done) => { - // Arrange - const privateOrganizationDetailsSubject = service["_organizationDetailsSubject"]; - const privateUserIdSubject = service["_userIdSubject"]; - - // Set up organization and user context - privateOrganizationDetailsSubject.next({ - organizationId: mockOrgId, - organizationName: mockOrgName, - }); - privateUserIdSubject.next(mockUserId); - - // Act - service.fetchReport(); - - // Assert - service.rawReportData$.subscribe((state) => { - if (!state.loading) { - expect(mockReportService.getRiskInsightsReport$).toHaveBeenCalledWith( - mockOrgId, - mockUserId, - ); - expect(state.data).toEqual(reportState); - done(); - } - }); - }); - it("should emit error ReportState when getRiskInsightsReport$ throws", (done) => { // Setup error passed via constructor for this test case mockReportService.getRiskInsightsReport$ = jest @@ -157,9 +129,8 @@ describe("RiskInsightsOrchestratorService", () => { organizationName: mockOrgName, }); _userIdSubject.next(mockUserId); - testService.fetchReport(); testService.rawReportData$.subscribe((state) => { - if (!state.loading) { + if (state.status != ReportStatus.Loading) { expect(state.error).toBe("Failed to fetch report"); expect(state.data).toBeNull(); done(); @@ -199,7 +170,7 @@ describe("RiskInsightsOrchestratorService", () => { // Assert service.rawReportData$.subscribe((state) => { - if (!state.loading && state.data) { + if (state.status != ReportStatus.Loading && state.data) { expect(mockMemberCipherDetailsApiService.getMemberCipherDetails).toHaveBeenCalledWith( mockOrgId, ); diff --git a/bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/domain/risk-insights-orchestrator.service.ts b/bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/domain/risk-insights-orchestrator.service.ts index 472aa7f5f6..650726628c 100644 --- a/bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/domain/risk-insights-orchestrator.service.ts +++ b/bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/domain/risk-insights-orchestrator.service.ts @@ -54,6 +54,7 @@ import { MemberDetails, OrganizationReportApplication, OrganizationReportSummary, + ReportStatus, ReportState, } from "../../models/report-models"; import { MemberCipherDetailsApiService } from "../api/member-cipher-details-api.service"; @@ -79,13 +80,16 @@ export class RiskInsightsOrchestratorService { } | null>(null); organizationDetails$ = this._organizationDetailsSubject.asObservable(); - // ------------------------- Raw data ------------------------- + // ------------------------- Cipher data ------------------------- private _ciphersSubject = new BehaviorSubject(null); private _ciphers$ = this._ciphersSubject.asObservable(); + private _hasCiphersSubject$ = new BehaviorSubject(null); + hasCiphers$ = this._hasCiphersSubject$.asObservable(); + // ------------------------- Report Variables ---------------- private _rawReportDataSubject = new BehaviorSubject({ - loading: true, + status: ReportStatus.Initializing, error: null, data: null, }); @@ -116,14 +120,10 @@ export class RiskInsightsOrchestratorService { // --------------------------- Critical Application data --------------------- criticalReportResults$: Observable = of(null); - // --------------------------- Vault Items Check --------------------- - hasVaultItems$: Observable = of(false); - // --------------------------- Trigger subjects --------------------- private _initializeOrganizationTriggerSubject = new Subject(); - private _fetchReportTriggerSubject = new Subject(); - private _markUnmarkUpdatesSubject = new Subject(); - private _markUnmarkUpdates$ = this._markUnmarkUpdatesSubject.asObservable(); + private _flagForUpdatesSubject = new Subject(); + private _flagForUpdates$ = this._flagForUpdatesSubject.asObservable(); private _reportStateSubscription: Subscription | null = null; private _migrationSubscription: Subscription | null = null; @@ -144,7 +144,6 @@ export class RiskInsightsOrchestratorService { this._setupCriticalApplicationContext(); this._setupCriticalApplicationReport(); this._setupEnrichedReportData(); - this._setupHasVaultItems(); this._setupInitializationPipeline(); this._setupMigrationAndCleanup(); this._setupReportState(); @@ -163,14 +162,6 @@ export class RiskInsightsOrchestratorService { this._destroy$.complete(); } - /** - * Fetches the latest report for the current organization and user - */ - fetchReport(): void { - this.logService.debug("[RiskInsightsOrchestratorService] Fetch report triggered"); - this._fetchReportTriggerSubject.next(); - } - /** * Generates a new report for the current organization and user */ @@ -187,7 +178,6 @@ export class RiskInsightsOrchestratorService { initializeForOrganization(organizationId: OrganizationId) { this.logService.debug("[RiskInsightsOrchestratorService] Initializing for org", organizationId); this._initializeOrganizationTriggerSubject.next(organizationId); - this.fetchReport(); } /** @@ -202,7 +192,7 @@ export class RiskInsightsOrchestratorService { ); return this.rawReportData$.pipe( take(1), - filter((data) => !data.loading && data.data != null), + filter((data) => data.status != ReportStatus.Loading && data.data != null), withLatestFrom( this.organizationDetails$.pipe(filter((org) => !!org && !!org.organizationId)), this._userId$.pipe(filter((userId) => !!userId)), @@ -311,9 +301,8 @@ export class RiskInsightsOrchestratorService { return forkJoin([updateApplicationsCall, updateSummaryCall]).pipe( map(() => updatedState), tap((finalState) => { - this._markUnmarkUpdatesSubject.next({ + this._flagForUpdatesSubject.next({ ...finalState, - organizationId: reportState.organizationId, }); }), catchError((error: unknown) => { @@ -331,7 +320,7 @@ export class RiskInsightsOrchestratorService { ); return this.rawReportData$.pipe( take(1), - filter((data) => !data.loading && data.data != null), + filter((data) => data.status != ReportStatus.Loading && data.data != null), withLatestFrom( this.organizationDetails$.pipe(filter((org) => !!org && !!org.organizationId)), this._userId$.pipe(filter((userId) => !!userId)), @@ -440,9 +429,8 @@ export class RiskInsightsOrchestratorService { return forkJoin([updateApplicationsCall, updateSummaryCall]).pipe( map(() => updatedState), tap((finalState) => { - this._markUnmarkUpdatesSubject.next({ + this._flagForUpdatesSubject.next({ ...finalState, - organizationId: reportState.organizationId, }); }), catchError((error: unknown) => { @@ -470,7 +458,7 @@ export class RiskInsightsOrchestratorService { return this.rawReportData$.pipe( take(1), - filter((data) => !data.loading && data.data != null), + filter((data) => data.status != ReportStatus.Loading && data.data != null), withLatestFrom( this.organizationDetails$.pipe(filter((org) => !!org && !!org.organizationId)), this._userId$.pipe(filter((userId) => !!userId)), @@ -545,9 +533,6 @@ export class RiskInsightsOrchestratorService { ) .pipe( map(() => updatedState), - tap((finalState) => { - this._markUnmarkUpdatesSubject.next(finalState); - }), catchError((error: unknown) => { this.logService.error( "[RiskInsightsOrchestratorService] Failed to save review status", @@ -565,16 +550,20 @@ export class RiskInsightsOrchestratorService { tap(() => this.logService.debug("[RiskInsightsOrchestratorService] Fetching report")), map((result): ReportState => { return { - loading: false, + status: ReportStatus.Complete, error: null, - data: result ?? null, - organizationId, + data: result, }; }), - catchError(() => - of({ loading: false, error: "Failed to fetch report", data: null, organizationId }), - ), - startWith({ loading: true, error: null, data: null, organizationId }), + catchError((error: unknown) => { + this.logService.error("[RiskInsightsOrchestratorService] Failed to fetch report", error); + return of({ + status: ReportStatus.Error, + error: "Failed to fetch report", + data: null, + organizationId, + }); + }), ); } @@ -650,7 +639,7 @@ export class RiskInsightsOrchestratorService { map((mappedResult): ReportState => { const { id, report, summary, applications, contentEncryptionKey } = mappedResult; return { - loading: false, + status: ReportStatus.Complete, error: null, data: { id, @@ -660,18 +649,20 @@ export class RiskInsightsOrchestratorService { creationDate: new Date(), contentEncryptionKey, }, - organizationId, }; }), catchError((): Observable => { return of({ - loading: false, + status: ReportStatus.Error, error: "Failed to generate or save report", data: null, - organizationId, }); }), - startWith({ loading: true, error: null, data: null, organizationId }), + startWith({ + status: ReportStatus.Loading, + error: null, + data: null, + }), ); } @@ -879,34 +870,6 @@ export class RiskInsightsOrchestratorService { } // Setup the pipeline to load critical applications when organization or user changes - /** - * Sets up an observable to check if the organization has any vault items (ciphers). - * This is used to determine which empty state to show in the UI. - */ - private _setupHasVaultItems() { - this.hasVaultItems$ = this.organizationDetails$.pipe( - switchMap((orgDetails) => { - if (!orgDetails?.organizationId) { - return of(false); - } - return from( - this.cipherService.getAllFromApiForOrganization(orgDetails.organizationId), - ).pipe( - map((ciphers) => ciphers.length > 0), - catchError((error: unknown) => { - this.logService.error( - "[RiskInsightsOrchestratorService] Error checking vault items", - error, - ); - return of(false); - }), - ); - }), - shareReplay({ bufferSize: 1, refCount: true }), - takeUntil(this._destroy$), - ); - } - private _setupCriticalApplicationContext() { this.organizationDetails$ .pipe( @@ -1003,6 +966,7 @@ export class RiskInsightsOrchestratorService { orgDetails.organizationId, ); this._ciphersSubject.next(ciphers); + this._hasCiphersSubject$.next(ciphers.length > 0); }), takeUntil(this._destroy$), ) @@ -1052,28 +1016,28 @@ export class RiskInsightsOrchestratorService { this._userId$.pipe(filter((user) => !!user)), ]).pipe(shareReplay({ bufferSize: 1, refCount: true })); - // A stream for the initial report fetch - const initialReportLoad$ = reportDependencies$.pipe( - take(1), - exhaustMap(([orgDetails, userId]) => this._fetchReport$(orgDetails!.organizationId, userId!)), - ); - - // A stream for manually triggered fetches - const manualReportFetch$ = this._fetchReportTriggerSubject.pipe( - withLatestFrom(reportDependencies$), - exhaustMap(([_, [orgDetails, userId]]) => - this._fetchReport$(orgDetails!.organizationId, userId!), + // A stream that continuously watches dependencies and fetches a new report every time they change + const continuousReportFetch$: Observable = reportDependencies$.pipe( + switchMap(([orgDetails, userId]) => + this._fetchReport$(orgDetails!.organizationId, userId!).pipe( + startWith({ status: ReportStatus.Initializing, error: null, data: null }), + ), ), ); // A stream for generating a new report - const newReportGeneration$ = this.generatingReport$.pipe( + const newReportGeneration$: Observable = this.generatingReport$.pipe( distinctUntilChanged(), filter((isRunning) => isRunning), withLatestFrom(reportDependencies$), exhaustMap(([_, [orgDetails, userId]]) => this._generateNewApplicationsReport$(orgDetails!.organizationId, userId!), ), + startWith({ + status: ReportStatus.Loading, + error: null, + data: null, + }), tap(() => { this._generateReportTriggerSubject.next(false); }), @@ -1081,30 +1045,44 @@ export class RiskInsightsOrchestratorService { // Combine all triggers and update the single report state const mergedReportState$ = merge( - initialReportLoad$, - manualReportFetch$, + continuousReportFetch$, newReportGeneration$, - this._markUnmarkUpdates$, + this._flagForUpdates$, ).pipe( - scan((prevState: ReportState, currState: ReportState) => { - // If organization changed, use new state completely (don't preserve old data) - // This allows null data to clear old org's data when switching orgs - if (currState.organizationId && prevState.organizationId !== currState.organizationId) { - return { - ...currState, - data: currState.data, // Allow null to clear old org's data - }; - } - - // Same org (or no org ID): preserve data when currState.data is null - // This preserves critical flags during loading states within the same org + startWith({ + status: ReportStatus.Initializing, + error: null, + data: null, + }), + withLatestFrom(this.organizationDetails$), + map(([reportState, orgDetails]) => { return { - ...prevState, - ...currState, - data: currState.data !== null ? currState.data : prevState.data, + reportState, + organizationId: orgDetails?.organizationId, + }; + }), + + // 3. NOW, scan receives a simple object for both prevState and currState + scan((prevState, currState) => { + const hasOrganizationChanged = prevState.organizationId !== currState.organizationId; + // Don't override initial status until complete + const keepInitializeStatus = + prevState.reportState.status == ReportStatus.Initializing && + currState.reportState.status == ReportStatus.Loading; + return { + reportState: { + status: keepInitializeStatus + ? prevState.reportState.status + : (currState.reportState.status ?? prevState.reportState.status), + error: currState.reportState.error ?? prevState.reportState.error, + data: + currState.reportState.data !== null || hasOrganizationChanged + ? currState.reportState.data + : prevState.reportState.data, + }, + organizationId: currState.organizationId, }; }), - startWith({ loading: false, error: null, data: null }), shareReplay({ bufferSize: 1, refCount: true }), takeUntil(this._destroy$), ); @@ -1112,7 +1090,7 @@ export class RiskInsightsOrchestratorService { this._reportStateSubscription = mergedReportState$ .pipe(takeUntil(this._destroy$)) .subscribe((state) => { - this._rawReportDataSubject.next(state); + this._rawReportDataSubject.next(state.reportState); }); } diff --git a/bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/domain/risk-insights-report.service.ts b/bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/domain/risk-insights-report.service.ts index 5ea66a7122..d49d7a4a40 100644 --- a/bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/domain/risk-insights-report.service.ts +++ b/bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/domain/risk-insights-report.service.ts @@ -1,4 +1,4 @@ -import { catchError, EMPTY, from, map, Observable, switchMap, throwError } from "rxjs"; +import { catchError, EMPTY, from, map, Observable, of, switchMap, throwError } from "rxjs"; import { EncString } from "@bitwarden/common/key-management/crypto/models/enc-string"; import { OrganizationId, OrganizationReportId, UserId } from "@bitwarden/common/types/guid"; @@ -146,12 +146,12 @@ export class RiskInsightsReportService { getRiskInsightsReport$( organizationId: OrganizationId, userId: UserId, - ): Observable { + ): Observable { return this.riskInsightsApiService.getRiskInsightsReport$(organizationId).pipe( switchMap((response) => { if (!response) { // Return an empty report and summary if response is falsy - return EMPTY; + return of(null as unknown as RiskInsightsData); } if (!response.contentEncryptionKey || response.contentEncryptionKey.data == "") { return throwError(() => new Error("Report key not found")); diff --git a/bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/view/risk-insights-data.service.ts b/bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/view/risk-insights-data.service.ts index a827200001..2dc669f572 100644 --- a/bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/view/risk-insights-data.service.ts +++ b/bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/view/risk-insights-data.service.ts @@ -4,7 +4,13 @@ import { distinctUntilChanged, map } from "rxjs/operators"; import { OrganizationId } from "@bitwarden/common/types/guid"; import { getAtRiskApplicationList, getAtRiskMemberList } from "../../helpers"; -import { ReportState, DrawerDetails, DrawerType, RiskInsightsEnrichedData } from "../../models"; +import { + ReportState, + DrawerDetails, + DrawerType, + RiskInsightsEnrichedData, + ReportStatus, +} from "../../models"; import { RiskInsightsOrchestratorService } from "../domain/risk-insights-orchestrator.service"; export class RiskInsightsDataService { @@ -24,11 +30,12 @@ export class RiskInsightsDataService { // -------------------------- Orchestrator-driven state ------------- // The full report state (for internal facade use or complex components) private readonly reportState$: Observable; - readonly isLoading$: Observable = of(false); + readonly reportStatus$: Observable = of(ReportStatus.Initializing); + readonly hasReportData$: Observable = of(false); readonly enrichedReportData$: Observable = of(null); readonly isGeneratingReport$: Observable = of(false); readonly criticalReportResults$: Observable = of(null); - readonly hasVaultItems$: Observable = of(false); + readonly hasCiphers$: Observable = of(null); // New applications that need review (reviewedDate === null) readonly newApplications$: Observable = of([]); @@ -52,14 +59,19 @@ export class RiskInsightsDataService { this.organizationDetails$ = this.orchestrator.organizationDetails$; this.enrichedReportData$ = this.orchestrator.enrichedReportData$; this.criticalReportResults$ = this.orchestrator.criticalReportResults$; - this.hasVaultItems$ = this.orchestrator.hasVaultItems$; this.newApplications$ = this.orchestrator.newApplications$; + this.hasCiphers$ = this.orchestrator.hasCiphers$.pipe(distinctUntilChanged()); + // Expose the loading state - this.isLoading$ = this.reportState$.pipe( - map((state) => state.loading), + this.reportStatus$ = this.reportState$.pipe( + map((state) => state.status), distinctUntilChanged(), // Prevent unnecessary component re-renders ); + this.hasReportData$ = this.reportState$.pipe( + map((state) => state.data != null), + distinctUntilChanged(), + ); } destroy(): void { @@ -76,10 +88,6 @@ export class RiskInsightsDataService { this.orchestrator.generateReport(); } - fetchReport(): void { - this.orchestrator.fetchReport(); - } - // ------------------------- Drawer functions ----------------------------- isActiveDrawerType = (drawerType: DrawerType): boolean => { return this.drawerDetailsSubject.value.activeDrawerType === drawerType; diff --git a/bitwarden_license/bit-web/src/app/dirt/access-intelligence/activity/all-activity.component.html b/bitwarden_license/bit-web/src/app/dirt/access-intelligence/activity/all-activity.component.html index 9fffded215..82137c9acc 100644 --- a/bitwarden_license/bit-web/src/app/dirt/access-intelligence/activity/all-activity.component.html +++ b/bitwarden_license/bit-web/src/app/dirt/access-intelligence/activity/all-activity.component.html @@ -1,4 +1,4 @@ -@if (dataService.isLoading$ | async) { +@if ((dataService.reportStatus$ | async) == ReportStatusEnum.Loading) { } @else {
    } @else { @let drawerDetails = dataService.drawerDetails$ | async; diff --git a/bitwarden_license/bit-web/src/app/dirt/access-intelligence/all-applications/all-applications.component.ts b/bitwarden_license/bit-web/src/app/dirt/access-intelligence/all-applications/all-applications.component.ts index 279ddc5e6f..b1cf43b211 100644 --- a/bitwarden_license/bit-web/src/app/dirt/access-intelligence/all-applications/all-applications.component.ts +++ b/bitwarden_license/bit-web/src/app/dirt/access-intelligence/all-applications/all-applications.component.ts @@ -10,7 +10,10 @@ import { RiskInsightsDataService, } from "@bitwarden/bit-common/dirt/reports/risk-insights"; import { createNewSummaryData } from "@bitwarden/bit-common/dirt/reports/risk-insights/helpers"; -import { OrganizationReportSummary } from "@bitwarden/bit-common/dirt/reports/risk-insights/models/report-models"; +import { + OrganizationReportSummary, + ReportStatus, +} from "@bitwarden/bit-common/dirt/reports/risk-insights/models/report-models"; import { Organization } from "@bitwarden/common/admin-console/models/domain/organization"; import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; import { @@ -53,6 +56,7 @@ export class AllApplicationsComponent implements OnInit { noItemsIcon = Security; protected markingAsCritical = false; protected applicationSummary: OrganizationReportSummary = createNewSummaryData(); + protected ReportStatusEnum = ReportStatus; destroyRef = inject(DestroyRef); diff --git a/bitwarden_license/bit-web/src/app/dirt/access-intelligence/critical-applications/critical-applications.component.html b/bitwarden_license/bit-web/src/app/dirt/access-intelligence/critical-applications/critical-applications.component.html index f0cfd06965..0e75758285 100644 --- a/bitwarden_license/bit-web/src/app/dirt/access-intelligence/critical-applications/critical-applications.component.html +++ b/bitwarden_license/bit-web/src/app/dirt/access-intelligence/critical-applications/critical-applications.component.html @@ -1,12 +1,4 @@ -
    - - {{ "loading" | i18n }} -
    -
    +

    {{ "criticalApplications" | i18n }}

    - - - - -
    - } -
    - -
    - @if (shouldShowTabs) { - - @if (isRiskInsightsActivityTabFeatureEnabled) { - - - - } - - - - - - - {{ - "criticalApplicationsWithCount" - | i18n: (dataService.criticalReportResults$ | async)?.reportData?.length ?? 0 - }} - - - - - } @else { -
    + @let status = dataService.reportStatus$ | async; + @let hasCiphers = dataService.hasCiphers$ | async; + @if (status == ReportStatusEnum.Initializing || hasCiphers === null) { + + + } @else { + + @if (!(dataService.hasReportData$ | async)) { +
    + @if (!hasCiphers) { + + } @else { + + + } +
    + } @else { + +
    +
    +

    {{ "riskInsights" | i18n }}

    +
    + {{ "reviewAtRiskPasswords" | i18n }} +
    + @if (dataLastUpdated) { +
    + + {{ + "dataLastUpdated" | i18n: (dataLastUpdated | date: "MMMM d, y 'at' h:mm a") + }} + @let isRunningReport = dataService.isGeneratingReport$ | async; + + + + + + +
    + }
    - } -
    -
    + +
    + + @if (isRiskInsightsActivityTabFeatureEnabled) { + + + + } + + + + + + + {{ + "criticalApplicationsWithCount" + | i18n: (dataService.criticalReportResults$ | async)?.reportData?.length ?? 0 + }} + + + + +
    +
    + } + } @if (dataService.drawerDetails$ | async; as drawerDetails) { diff --git a/bitwarden_license/bit-web/src/app/dirt/access-intelligence/risk-insights.component.ts b/bitwarden_license/bit-web/src/app/dirt/access-intelligence/risk-insights.component.ts index 5bd5096770..cde5d5c8c6 100644 --- a/bitwarden_license/bit-web/src/app/dirt/access-intelligence/risk-insights.component.ts +++ b/bitwarden_license/bit-web/src/app/dirt/access-intelligence/risk-insights.component.ts @@ -8,6 +8,7 @@ import { map, tap } from "rxjs/operators"; import { JslibModule } from "@bitwarden/angular/jslib.module"; import { DrawerType, + ReportStatus, RiskInsightsDataService, } from "@bitwarden/bit-common/dirt/reports/risk-insights"; import { FeatureFlag } from "@bitwarden/common/enums/feature-flag.enum"; @@ -29,6 +30,7 @@ import { AllApplicationsComponent } from "./all-applications/all-applications.co import { CriticalApplicationsComponent } from "./critical-applications/critical-applications.component"; import { EmptyStateCardComponent } from "./empty-state-card.component"; import { RiskInsightsTabType } from "./models/risk-insights.models"; +import { ApplicationsLoadingComponent } from "./shared/risk-insights-loading.component"; // FIXME(https://bitwarden.atlassian.net/browse/CL-764): Migrate to OnPush // eslint-disable-next-line @angular-eslint/prefer-on-push-component-change-detection @@ -48,41 +50,35 @@ import { RiskInsightsTabType } from "./models/risk-insights.models"; DrawerBodyComponent, DrawerHeaderComponent, AllActivityComponent, + ApplicationsLoadingComponent, ], }) export class RiskInsightsComponent implements OnInit, OnDestroy { private destroyRef = inject(DestroyRef); private _isDrawerOpen: boolean = false; + protected ReportStatusEnum = ReportStatus; tabIndex: RiskInsightsTabType = RiskInsightsTabType.AllApps; isRiskInsightsActivityTabFeatureEnabled: boolean = false; appsCount: number = 0; - // Leaving this commented because it's not used but seems important - // notifiedMembersCount: number = 0; private organizationId: OrganizationId = "" as OrganizationId; dataLastUpdated: Date | null = null; - refetching: boolean = false; // Empty state properties - protected hasReportBeenRun = false; - protected reportHasLoaded = false; - protected hasVaultItems = false; - private organizationName = ""; + protected organizationName = ""; // Empty state computed properties - protected shouldShowImportDataState = false; - protected emptyStateTitle = ""; - protected emptyStateDescription = ""; - protected emptyStateBenefits: [string, string][] = []; - protected emptyStateButtonText = ""; - protected emptyStateButtonIcon = ""; - protected emptyStateButtonAction: (() => void) | null = null; + protected emptyStateBenefits: [string, string][] = [ + [this.i18nService.t("benefit1Title"), this.i18nService.t("benefit1Description")], + [this.i18nService.t("benefit2Title"), this.i18nService.t("benefit2Description")], + [this.i18nService.t("benefit3Title"), this.i18nService.t("benefit3Description")], + ]; protected emptyStateVideoSrc: string | null = "/videos/risk-insights-mark-as-critical.mp4"; - private static readonly IMPORT_ICON = "bwi bwi-download"; + protected IMPORT_ICON = "bwi bwi-download"; // TODO: See https://github.com/bitwarden/clients/pull/16832#discussion_r2474523235 @@ -91,7 +87,7 @@ export class RiskInsightsComponent implements OnInit, OnDestroy { private router: Router, private configService: ConfigService, protected dataService: RiskInsightsDataService, - private i18nService: I18nService, + protected i18nService: I18nService, ) { this.route.queryParams.pipe(takeUntilDestroyed(this.destroyRef)).subscribe(({ tabIndex }) => { this.tabIndex = !isNaN(Number(tabIndex)) ? Number(tabIndex) : RiskInsightsTabType.AllApps; @@ -125,28 +121,15 @@ export class RiskInsightsComponent implements OnInit, OnDestroy { // Combine report data, vault items check, organization details, and generation state // This declarative pattern ensures proper cleanup and prevents memory leaks - combineLatest([ - this.dataService.enrichedReportData$, - this.dataService.hasVaultItems$, - this.dataService.organizationDetails$, - this.dataService.isGeneratingReport$, - ]) + combineLatest([this.dataService.enrichedReportData$, this.dataService.organizationDetails$]) .pipe(takeUntilDestroyed(this.destroyRef)) - .subscribe(([report, hasVaultItems, orgDetails, isGenerating]) => { + .subscribe(([report, orgDetails]) => { // Update report state - this.reportHasLoaded = true; - this.hasReportBeenRun = !!report?.creationDate; this.appsCount = report?.reportData.length ?? 0; this.dataLastUpdated = report?.creationDate ?? null; - // Update vault items state - this.hasVaultItems = hasVaultItems; - // Update organization name this.organizationName = orgDetails?.organizationName ?? ""; - - // Update all empty state properties based on current state - this.updateEmptyStateProperties(isGenerating); }); // Subscribe to drawer state changes @@ -171,10 +154,6 @@ export class RiskInsightsComponent implements OnInit, OnDestroy { } } - get shouldShowTabs(): boolean { - return this.appsCount > 0; - } - async onTabChange(newIndex: number): Promise { await this.router.navigate([], { relativeTo: this.route, @@ -228,37 +207,4 @@ export class RiskInsightsComponent implements OnInit, OnDestroy { "import", ]); }; - - /** - * Updates all empty state properties based on current state. - * Called whenever the underlying data (hasVaultItems, hasReportBeenRun, reportHasLoaded) changes. - */ - private updateEmptyStateProperties(isGenerating: boolean): void { - // Calculate boolean flags - // Note: We only show empty states when there are NO apps (appsCount === 0) - // The template uses @if(shouldShowTabs) to determine whether to show tabs or empty state - this.shouldShowImportDataState = !this.hasVaultItems && !isGenerating; - - // Update benefits (constant for all states) - this.emptyStateBenefits = [ - [this.i18nService.t("benefit1Title"), this.i18nService.t("benefit1Description")], - [this.i18nService.t("benefit2Title"), this.i18nService.t("benefit2Description")], - [this.i18nService.t("benefit3Title"), this.i18nService.t("benefit3Description")], - ]; - - // Update all state-dependent properties in single if/else - if (this.shouldShowImportDataState) { - this.emptyStateTitle = this.i18nService.t("noApplicationsInOrgTitle", this.organizationName); - this.emptyStateDescription = this.i18nService.t("noApplicationsInOrgDescription"); - this.emptyStateButtonText = this.i18nService.t("importData"); - this.emptyStateButtonIcon = RiskInsightsComponent.IMPORT_ICON; - this.emptyStateButtonAction = this.goToImportPage; - } else { - this.emptyStateTitle = this.i18nService.t("noReportRunTitle"); - this.emptyStateDescription = this.i18nService.t("noReportRunDescription"); - this.emptyStateButtonText = this.i18nService.t("riskInsightsRunReport"); - this.emptyStateButtonIcon = ""; - this.emptyStateButtonAction = this.generateReport.bind(this); - } - } }