mirror of
https://github.com/bitwarden/browser
synced 2026-02-03 02:03:53 +00:00
fix(dirt): add subscription cleanup and remove dead code
Critical fixes for production code quality and memory leak prevention: **Subscription Cleanup (ADR-0003 Compliance):** - Add takeUntil pattern to AllActivitiesService subscriptions - Add _destroy$ Subject and destroy() method - Prevents memory leaks by properly unsubscribing from observables - Follows Observable Data Services ADR requirements **Code Quality Improvements:** - Remove unused 'isSaving' variable from AllActivityComponent - Remove unused 'finalize' import - Add comment explaining error is already logged upstream - Cleaner, more maintainable code This ensures proper resource cleanup and follows Bitwarden's architectural decision records for observable management. Related to PM-27284
This commit is contained in:
@@ -1,4 +1,5 @@
|
||||
import { BehaviorSubject } from "rxjs";
|
||||
import { BehaviorSubject, Subject } from "rxjs";
|
||||
import { takeUntil } from "rxjs/operators";
|
||||
|
||||
import { ApplicationHealthReportDetailEnriched } from "../../models";
|
||||
import { OrganizationReportSummary } from "../../models/report-models";
|
||||
@@ -11,6 +12,8 @@ export class AllActivitiesService {
|
||||
/// Going forward, this class can be simplified by using the RiskInsightsDataService
|
||||
/// as it contains the application summary data.
|
||||
|
||||
private _destroy$ = new Subject<void>();
|
||||
|
||||
private reportSummarySubject$ = new BehaviorSubject<OrganizationReportSummary>({
|
||||
totalMemberCount: 0,
|
||||
totalCriticalMemberCount: 0,
|
||||
@@ -40,7 +43,7 @@ export class AllActivitiesService {
|
||||
|
||||
constructor(private dataService: RiskInsightsDataService) {
|
||||
// All application summary changes
|
||||
this.dataService.enrichedReportData$.subscribe((report) => {
|
||||
this.dataService.enrichedReportData$.pipe(takeUntil(this._destroy$)).subscribe((report) => {
|
||||
if (report) {
|
||||
this.setAllAppsReportSummary(report.summaryData);
|
||||
this.setAllAppsReportDetails(report.reportData);
|
||||
@@ -48,14 +51,14 @@ export class AllActivitiesService {
|
||||
});
|
||||
|
||||
// Critical application summary changes
|
||||
this.dataService.criticalReportResults$.subscribe((report) => {
|
||||
this.dataService.criticalReportResults$.pipe(takeUntil(this._destroy$)).subscribe((report) => {
|
||||
if (report) {
|
||||
this.setCriticalAppsReportSummary(report.summaryData);
|
||||
}
|
||||
});
|
||||
|
||||
// New applications changes (from orchestrator's reactive pipeline)
|
||||
this.dataService.newApplications$.subscribe((newApps) => {
|
||||
this.dataService.newApplications$.pipe(takeUntil(this._destroy$)).subscribe((newApps) => {
|
||||
this.setNewApplications(newApps);
|
||||
});
|
||||
}
|
||||
@@ -108,4 +111,13 @@ export class AllActivitiesService {
|
||||
newApplications: newApps,
|
||||
});
|
||||
}
|
||||
|
||||
/**
|
||||
* Cleanup method to unsubscribe from all observables.
|
||||
* Should be called when the service is destroyed to prevent memory leaks.
|
||||
*/
|
||||
destroy(): void {
|
||||
this._destroy$.next();
|
||||
this._destroy$.complete();
|
||||
}
|
||||
}
|
||||
|
||||
@@ -2,7 +2,6 @@ import { Component, DestroyRef, inject, OnInit } from "@angular/core";
|
||||
import { takeUntilDestroyed } from "@angular/core/rxjs-interop";
|
||||
import { ActivatedRoute } from "@angular/router";
|
||||
import { firstValueFrom } from "rxjs";
|
||||
import { finalize } from "rxjs/operators";
|
||||
|
||||
import {
|
||||
AllActivitiesService,
|
||||
@@ -99,15 +98,7 @@ export class AllActivityComponent implements OnInit {
|
||||
|
||||
if (result?.saved) {
|
||||
// User clicked Continue - save review status and critical flags
|
||||
let isSaving = true;
|
||||
|
||||
firstValueFrom(
|
||||
this.dataService.saveApplicationReviewStatus(result.selectedApplications).pipe(
|
||||
finalize(() => {
|
||||
isSaving = false;
|
||||
}),
|
||||
),
|
||||
)
|
||||
firstValueFrom(this.dataService.saveApplicationReviewStatus(result.selectedApplications))
|
||||
.then(() => {
|
||||
// Success - data will update automatically through reactive pipeline
|
||||
if (result.selectedApplications.length > 0) {
|
||||
@@ -127,7 +118,8 @@ export class AllActivityComponent implements OnInit {
|
||||
});
|
||||
}
|
||||
})
|
||||
.catch((error) => {
|
||||
.catch(() => {
|
||||
// Error already logged by orchestrator service
|
||||
this.toastService.showToast({
|
||||
variant: "error",
|
||||
title: this.i18nService.t("error"),
|
||||
|
||||
Reference in New Issue
Block a user