bitwarden/browser
1
0
Fork 0
mirror of https://github.com/bitwarden/browser synced 2025-12-21 18:53:29 +00:00
Code Issues Releases Wiki Activity

[PM-27694] Handle empty report response (#17162)

Browse Source 
* 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
This commit is contained in:
Leslie Tilton
2025-10-31 13:59:11 -05:00
committed by GitHub
parent 4908d73804
commit 23cb3e092c
14 changed files with 237 additions and 303 deletions
Download Patch File Download Diff File Expand all files Collapse all files

12
bitwarden_license/bit-common/src/dirt/reports/risk-insights/models/report-models.ts
View File

@@ -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

4
bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/api/risk-insights-api.service.ts
View File

@@ -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

35
bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/domain/risk-insights-orchestrator.service.spec.ts
View File

@@ -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,
);

178
bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/domain/risk-insights-orchestrator.service.ts
View File

@@ -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<CipherView[] | null>(null);
private _ciphers$ = this._ciphersSubject.asObservable();
private _hasCiphersSubject$ = new BehaviorSubject<boolean | null>(null);
hasCiphers$ = this._hasCiphersSubject$.asObservable();
// ------------------------- Report Variables ----------------
private _rawReportDataSubject = new BehaviorSubject<ReportState>({
loading: true,
status: ReportStatus.Initializing,
error: null,
data: null,
});
@@ -116,14 +120,10 @@ export class RiskInsightsOrchestratorService {
// --------------------------- Critical Application data ---------------------
criticalReportResults$: Observable<RiskInsightsEnrichedData | null> = of(null);
// --------------------------- Vault Items Check ---------------------
hasVaultItems$: Observable<boolean> = of(false);
// --------------------------- Trigger subjects ---------------------
private _initializeOrganizationTriggerSubject = new Subject<OrganizationId>();
private _fetchReportTriggerSubject = new Subject<void>();
private _markUnmarkUpdatesSubject = new Subject<ReportState>();
private _markUnmarkUpdates$ = this._markUnmarkUpdatesSubject.asObservable();
private _flagForUpdatesSubject = new Subject<ReportState>();
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<ReportState> => {
return of({
loading: false,
status: ReportStatus.Error,
error: "Failed to generate or save report",
data: null,
organizationId,
});
}),
startWith<ReportState>({ loading: true, error: null, data: null, organizationId }),
startWith<ReportState>({
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<ReportState> = reportDependencies$.pipe(
switchMap(([orgDetails, userId]) =>
this._fetchReport$(orgDetails!.organizationId, userId!).pipe(
startWith<ReportState>({ status: ReportStatus.Initializing, error: null, data: null }),
),
),
);
// A stream for generating a new report
const newReportGeneration$ = this.generatingReport$.pipe(
const newReportGeneration$: Observable<ReportState> = this.generatingReport$.pipe(
distinctUntilChanged(),
filter((isRunning) => isRunning),
withLatestFrom(reportDependencies$),
exhaustMap(([_, [orgDetails, userId]]) =>
this._generateNewApplicationsReport$(orgDetails!.organizationId, userId!),
),
startWith<ReportState>({
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<ReportState>({
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);
});
}

6
bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/domain/risk-insights-report.service.ts
View File

@@ -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<RiskInsightsData> {
): Observable<RiskInsightsData | null> {
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"));

28
bitwarden_license/bit-common/src/dirt/reports/risk-insights/services/view/risk-insights-data.service.ts
View File

@@ -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<ReportState>;
readonly isLoading$: Observable<boolean> = of(false);
readonly reportStatus$: Observable<ReportStatus> = of(ReportStatus.Initializing);
readonly hasReportData$: Observable<boolean> = of(false);
readonly enrichedReportData$: Observable<RiskInsightsEnrichedData | null> = of(null);
readonly isGeneratingReport$: Observable<boolean> = of(false);
readonly criticalReportResults$: Observable<RiskInsightsEnrichedData | null> = of(null);
readonly hasVaultItems$: Observable<boolean> = of(false);
readonly hasCiphers$: Observable<boolean | null> = of(null);
// New applications that need review (reviewedDate === null)
readonly newApplications$: Observable<string[]> = 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;