Dirt/pm 30319/phish cache freeze

apps/browser/src/dirt/phishing-detection/services/phishing-data.service.ts

@@ -1,12 +1,13 @@
 import {
   catchError,
   EMPTY,
+  filter,
+  finalize,
   first,
   firstValueFrom,
   map,
   retry,
-  share,
-  startWith,
+  shareReplay,
   Subject,
   switchMap,
   tap,
@@ -62,14 +63,43 @@ export class PhishingDataService {
   // How often are new web addresses added to the remote?
   readonly UPDATE_INTERVAL_DURATION = 24 * 60 * 60 * 1000; // 24 hours
 
+  // Minimum time between updates when triggered by account switch (5 minutes)
+  private readonly MIN_UPDATE_INTERVAL = 5 * 60 * 1000;
+
   private _triggerUpdate$ = new Subject<void>();
+  private _updateInProgress = false;
+
+  /**
+   * Observable that handles phishing data updates.
+   *
+   * Updates are triggered explicitly via triggerUpdateIfNeeded() or the 24-hour scheduler.
+   * The observable includes safeguards to prevent redundant updates:
+   * - Skips if an update is already in progress
+   * - Skips if cache was updated within MIN_UPDATE_INTERVAL (5 min)
+   */
   update$ = this._triggerUpdate$.pipe(
-    startWith(undefined), // Always emit once
+    // Don't use startWith - initial update is handled by triggerUpdateIfNeeded()
     tap(() => this.logService.info(`[PhishingDataService] Update triggered...`)),
+    filter(() => {
+      if (this._updateInProgress) {
+        this.logService.info(`[PhishingDataService] Update already in progress, skipping...`);
+        return false;
+      }
+      return true;
+    }),
+    tap(() => {
+      this._updateInProgress = true;
+    }),
     switchMap(() =>
       this._cachedState.state$.pipe(
         first(), // Only take the first value to avoid an infinite loop when updating the cache below
         switchMap(async (cachedState) => {
+          // Early exit if cache is fresh (updated within MIN_UPDATE_INTERVAL)
+          if (cachedState && Date.now() - cachedState.timestamp < this.MIN_UPDATE_INTERVAL) {
+            this.logService.info(`[PhishingDataService] Cache is fresh, skipping update...`);
+            return;
+          }
+
           const next = await this.getNextWebAddresses(cachedState);
           if (next) {
             await this._cachedState.update(() => next);
@@ -98,9 +128,14 @@ export class PhishingDataService {
             return EMPTY;
           },
         ),
+        // Use finalize() to ensure _updateInProgress is reset on success, error, OR completion
+        // Per ADR: "Use finalize() operator to ensure cleanup code always runs"
+        finalize(() => {
+          this._updateInProgress = false;
+        }),
       ),
     ),
-    share(),
+    shareReplay({ bufferSize: 1, refCount: true }),
   );
 
   constructor(
@@ -120,6 +155,14 @@ export class PhishingDataService {
     );
   }
 
+  /**
+   * Triggers an update if the cache is stale or empty.
+   * Should be called when phishing detection is enabled for an account.
+   */
+  triggerUpdateIfNeeded(): void {
+    this._triggerUpdate$.next();
+  }
+
   /**
    * Checks if the given URL is a known phishing web address
    *

apps/browser/src/dirt/phishing-detection/services/phishing-detection.service.spec.ts

@@ -1,5 +1,5 @@
 import { mock, MockProxy } from "jest-mock-extended";
-import { Observable, of } from "rxjs";
+import { EMPTY, Observable, of } from "rxjs";
 
 import { PhishingDetectionSettingsServiceAbstraction } from "@bitwarden/common/dirt/services/abstractions/phishing-detection-settings.service.abstraction";
 import { LogService } from "@bitwarden/common/platform/abstractions/log.service";
@@ -16,7 +16,9 @@ describe("PhishingDetectionService", () => {
 
   beforeEach(() => {
     logService = { info: jest.fn(), debug: jest.fn(), warning: jest.fn(), error: jest.fn() } as any;
-    phishingDataService = mock();
+    phishingDataService = mock<PhishingDataService>({
+      update$: EMPTY,
+    });
     messageListener = mock<MessageListener>({
       messages$(_commandDefinition) {
         return new Observable();

apps/browser/src/dirt/phishing-detection/services/phishing-detection.service.ts

@@ -1,10 +1,12 @@
 import {
   concatMap,
+  delay,
   distinctUntilChanged,
   EMPTY,
   filter,
   map,
   merge,
+  of,
   Subject,
   switchMap,
   tap,
@@ -113,6 +115,10 @@ export class PhishingDetectionService {
 
     const phishingDetectionActive$ = phishingDetectionSettingsService.on$;
 
+    // Subscribe to update$ once at initialization - runs in background, doesn't block popup
+    // This subscription lives for the lifetime of the service worker
+    const updateSub = phishingDataService.update$.subscribe();
+
     const initSub = phishingDetectionActive$
       .pipe(
         distinctUntilChanged(),
@@ -124,19 +130,22 @@ export class PhishingDetectionService {
             return EMPTY;
           } else {
             logService.debug("[PhishingDetectionService] Enabling phishing detection service");
-            return merge(
-              phishingDataService.update$,
-              onContinueCommand$,
-              onTabUpdated$,
-              onCancelCommand$,
-            );
+            // Trigger cache update asynchronously using RxJS delay(0)
+            // This defers to the next event loop tick, preventing UI blocking during account switch
+            of(null)
+              .pipe(delay(0))
+              .subscribe(() => phishingDataService.triggerUpdateIfNeeded());
+            // update$ removed from merge - popup no longer blocks waiting for update
+            // The actual update runs via updateSub above
+            return merge(onContinueCommand$, onTabUpdated$, onCancelCommand$);
           }
         }),
       )
       .subscribe();
 
     this._didInit = true;
     return () => {
+      updateSub.unsubscribe();
       initSub.unsubscribe();
       this._didInit = false;