Merge branch 'main' into vault/pm-30304/cipher-sdk-get

apps/browser/src/vault/popup/components/vault/add-edit/add-edit.component.spec.ts

@@ -1,4 +1,3 @@
-import { Location } from "@angular/common";
 import { ComponentFixture, fakeAsync, TestBed, tick } from "@angular/core/testing";
 import { By } from "@angular/platform-browser";
 import { provideNoopAnimations } from "@angular/platform-browser/animations";
@@ -39,6 +38,7 @@ import { BrowserApi } from "../../../../../platform/browser/browser-api";
 import BrowserPopupUtils from "../../../../../platform/browser/browser-popup-utils";
 import { PopupRouterCacheService } from "../../../../../platform/popup/view-cache/popup-router-cache.service";
 import { PopupCloseWarningService } from "../../../../../popup/services/popup-close-warning.service";
+import { VaultPopupAfterDeletionNavigationService } from "../../../services/vault-popup-after-deletion-navigation.service";
 
 import { AddEditComponent } from "./add-edit.component";
 
@@ -61,8 +61,7 @@ describe("AddEditComponent", () => {
   const back = jest.fn().mockResolvedValue(null);
   const setHistory = jest.fn();
   const collect = jest.fn().mockResolvedValue(null);
-  const history$ = jest.fn();
-  const historyGo = jest.fn().mockResolvedValue(null);
+  const navigateAfterDeletion = jest.fn().mockResolvedValue(undefined);
   const openSimpleDialog = jest.fn().mockResolvedValue(true);
   const cipherArchiveService = mock<CipherArchiveService>();
 
@@ -72,8 +71,7 @@ describe("AddEditComponent", () => {
     navigate.mockClear();
     back.mockClear();
     collect.mockClear();
-    history$.mockClear();
-    historyGo.mockClear();
+    navigateAfterDeletion.mockClear();
     openSimpleDialog.mockClear();
 
     cipherArchiveService.hasArchiveFlagEnabled$ = of(true);
@@ -90,10 +88,9 @@ describe("AddEditComponent", () => {
         provideNoopAnimations(),
         { provide: PlatformUtilsService, useValue: mock<PlatformUtilsService>() },
         { provide: ConfigService, useValue: mock<ConfigService>() },
-        { provide: PopupRouterCacheService, useValue: { back, setHistory, history$ } },
+        { provide: PopupRouterCacheService, useValue: { back, setHistory } },
         { provide: PopupCloseWarningService, useValue: { disable } },
         { provide: Router, useValue: { navigate } },
-        { provide: Location, useValue: { historyGo } },
         { provide: ActivatedRoute, useValue: { queryParams: queryParams$ } },
         { provide: I18nService, useValue: { t: (key: string) => key } },
         { provide: CipherService, useValue: cipherServiceMock },
@@ -129,6 +126,10 @@ describe("AddEditComponent", () => {
             unarchiveCipher: jest.fn().mockResolvedValue(null),
           },
         },
+        {
+          provide: VaultPopupAfterDeletionNavigationService,
+          useValue: { navigateAfterDeletion },
+        },
       ],
     })
       .overrideProvider(CipherFormConfigService, {
@@ -465,10 +466,10 @@ describe("AddEditComponent", () => {
       jest.spyOn(component["dialogService"], "openSimpleDialog").mockResolvedValue(true);
       await component.delete();
 
-      expect(navigate).toHaveBeenCalledWith(["/tabs/vault"]);
+      expect(navigateAfterDeletion).toHaveBeenCalledWith("/tabs/vault");
     });
 
-    it("navigates to custom route when not in history", fakeAsync(() => {
+    it("navigates to custom route after deletion", fakeAsync(() => {
       buildConfigResponse.originalCipher = { edit: true, id: "123" } as Cipher;
       queryParams$.next({
         cipherId: "123",
@@ -477,54 +478,15 @@ describe("AddEditComponent", () => {
 
       tick();
 
-      // Mock history without the target route
-      history$.mockReturnValue(
-        of([
-          { url: "/tabs/vault" },
-          { url: "/view-cipher?cipherId=123" },
-          { url: "/add-edit?cipherId=123" },
-        ]),
-      );
-
       jest.spyOn(component["dialogService"], "openSimpleDialog").mockResolvedValue(true);
 
       void component.delete();
       tick();
 
-      expect(history$).toHaveBeenCalled();
-      expect(historyGo).not.toHaveBeenCalled();
-      expect(navigate).toHaveBeenCalledWith(["/archive"]);
+      expect(navigateAfterDeletion).toHaveBeenCalledWith("/archive");
     }));
 
-    it("uses historyGo when custom route exists in history", fakeAsync(() => {
-      buildConfigResponse.originalCipher = { edit: true, id: "123" } as Cipher;
-      queryParams$.next({
-        cipherId: "123",
-        routeAfterDeletion: "/archive",
-      });
-
-      tick();
-
-      history$.mockReturnValue(
-        of([
-          { url: "/tabs/vault" },
-          { url: "/archive" },
-          { url: "/view-cipher?cipherId=123" },
-          { url: "/add-edit?cipherId=123" },
-        ]),
-      );
-
-      jest.spyOn(component["dialogService"], "openSimpleDialog").mockResolvedValue(true);
-
-      void component.delete();
-      tick();
-
-      expect(history$).toHaveBeenCalled();
-      expect(historyGo).toHaveBeenCalledWith(-2);
-      expect(navigate).not.toHaveBeenCalled();
-    }));
-
-    it("uses router.navigate for default /tabs/vault route", fakeAsync(() => {
+    it("uses default /tabs/vault route when routeAfterDeletion is not set", fakeAsync(() => {
       buildConfigResponse.originalCipher = { edit: true, id: "456" } as Cipher;
       component.routeAfterDeletion = "/tabs/vault";
 
@@ -539,9 +501,7 @@ describe("AddEditComponent", () => {
       void component.delete();
       tick();
 
-      expect(history$).not.toHaveBeenCalled();
-      expect(historyGo).not.toHaveBeenCalled();
-      expect(navigate).toHaveBeenCalledWith(["/tabs/vault"]);
+      expect(navigateAfterDeletion).toHaveBeenCalledWith("/tabs/vault");
     }));
 
     it("ignores invalid routeAfterDeletion query param and uses default route", fakeAsync(() => {

apps/browser/src/vault/popup/components/vault/add-edit/add-edit.component.ts

@@ -1,6 +1,6 @@
 // FIXME: Update this file to be type safe and remove this and next line
 // @ts-strict-ignore
-import { CommonModule, Location } from "@angular/common";
+import { CommonModule } from "@angular/common";
 import { Component, OnInit, OnDestroy, viewChild } from "@angular/core";
 import { takeUntilDestroyed } from "@angular/core/rxjs-interop";
 import { FormsModule } from "@angular/forms";
@@ -33,7 +33,6 @@ import {
   BadgeModule,
 } from "@bitwarden/components";
 import {
-  ArchiveCipherUtilitiesService,
   CipherFormComponent,
   CipherFormConfig,
   CipherFormConfigService,
@@ -57,6 +56,7 @@ import { PopupCloseWarningService } from "../../../../../popup/services/popup-cl
 import { BrowserCipherFormGenerationService } from "../../../services/browser-cipher-form-generation.service";
 import { BrowserPremiumUpgradePromptService } from "../../../services/browser-premium-upgrade-prompt.service";
 import { BrowserTotpCaptureService } from "../../../services/browser-totp-capture.service";
+import { VaultPopupAfterDeletionNavigationService } from "../../../services/vault-popup-after-deletion-navigation.service";
 import {
   fido2PopoutSessionData$,
   Fido2SessionData,
@@ -233,9 +233,8 @@ export class AddEditComponent implements OnInit, OnDestroy {
     private dialogService: DialogService,
     protected cipherAuthorizationService: CipherAuthorizationService,
     private accountService: AccountService,
-    private location: Location,
     private archiveService: CipherArchiveService,
-    private archiveCipherUtilsService: ArchiveCipherUtilitiesService,
+    private afterDeletionNavigationService: VaultPopupAfterDeletionNavigationService,
   ) {
     this.subscribeToParams();
   }
@@ -496,21 +495,7 @@ export class AddEditComponent implements OnInit, OnDestroy {
       return false;
     }
 
-    if (this.routeAfterDeletion !== ROUTES_AFTER_EDIT_DELETION.tabsVault) {
-      const history = await firstValueFrom(this.popupRouterCacheService.history$());
-      const targetIndex = history.map((h) => h.url).lastIndexOf(this.routeAfterDeletion);
-
-      if (targetIndex !== -1) {
-        const stepsBack = targetIndex - (history.length - 1);
-        // Use historyGo to navigate back to the target route in history
-        // This allows downstream calls to `back()` to continue working as expected
-        await this.location.historyGo(stepsBack);
-      } else {
-        await this.router.navigate([this.routeAfterDeletion]);
-      }
-    } else {
-      await this.router.navigate([this.routeAfterDeletion]);
-    }
+    await this.afterDeletionNavigationService.navigateAfterDeletion(this.routeAfterDeletion);
 
     this.toastService.showToast({
       variant: "success",

apps/browser/src/vault/popup/components/vault/view/view.component.spec.ts

@@ -47,8 +47,8 @@ import {
 import { BrowserApi } from "../../../../../platform/browser/browser-api";
 import BrowserPopupUtils from "../../../../../platform/browser/browser-popup-utils";
 import { PopupRouterCacheService } from "../../../../../platform/popup/view-cache/popup-router-cache.service";
+import { VaultPopupAfterDeletionNavigationService } from "../../../services/vault-popup-after-deletion-navigation.service";
 import { VaultPopupAutofillService } from "../../../services/vault-popup-autofill.service";
-import { VaultPopupScrollPositionService } from "../../../services/vault-popup-scroll-position.service";
 import {
   AutofillConfirmationDialogComponent,
   AutofillConfirmationDialogResult,
@@ -72,7 +72,7 @@ describe("ViewComponent", () => {
   const copy = jest.fn().mockResolvedValue(true);
   const back = jest.fn().mockResolvedValue(null);
   const openSimpleDialog = jest.fn().mockResolvedValue(true);
-  const stop = jest.fn();
+  const navigateAfterDeletion = jest.fn().mockResolvedValue(undefined);
   const showToast = jest.fn();
   const showPasswordPrompt = jest.fn().mockResolvedValue(true);
   const getFeatureFlag$ = jest.fn().mockReturnValue(of(true));
@@ -127,7 +127,7 @@ describe("ViewComponent", () => {
     doAutofill.mockClear();
     doAutofillAndSave.mockClear();
     copy.mockClear();
-    stop.mockClear();
+    navigateAfterDeletion.mockClear();
     openSimpleDialog.mockClear();
     back.mockClear();
     showToast.mockClear();
@@ -150,7 +150,10 @@ describe("ViewComponent", () => {
         { provide: PopupRouterCacheService, useValue: mock<PopupRouterCacheService>({ back }) },
         { provide: ActivatedRoute, useValue: { queryParams: params$ } },
         { provide: EventCollectionService, useValue: { collect } },
-        { provide: VaultPopupScrollPositionService, useValue: { stop } },
+        {
+          provide: VaultPopupAfterDeletionNavigationService,
+          useValue: { navigateAfterDeletion },
+        },
         { provide: VaultPopupAutofillService, useValue: mockVaultPopupAutofillService },
         { provide: ToastService, useValue: { showToast } },
         { provide: ConfigService, useValue: { getFeatureFlag$, getFeatureFlag } },
@@ -561,17 +564,10 @@ describe("ViewComponent", () => {
       expect(openSimpleDialog).toHaveBeenCalledTimes(1);
     });
 
-    it("navigates back", async () => {
+    it("navigates after deletion", async () => {
       await component.delete();
 
-      expect(back).toHaveBeenCalledTimes(1);
-    });
-
-    it("stops scroll position service", async () => {
-      await component.delete();
-
-      expect(stop).toHaveBeenCalledTimes(1);
-      expect(stop).toHaveBeenCalledWith(true);
+      expect(navigateAfterDeletion).toHaveBeenCalledTimes(1);
     });
 
     describe("deny confirmation", () => {
@@ -587,8 +583,7 @@ describe("ViewComponent", () => {
       });
 
       it("does not interact with side effects", () => {
-        expect(back).not.toHaveBeenCalled();
-        expect(stop).not.toHaveBeenCalled();
+        expect(navigateAfterDeletion).not.toHaveBeenCalled();
         expect(showToast).not.toHaveBeenCalled();
       });
     });

apps/browser/src/vault/popup/components/vault/view/view.component.ts

@@ -67,10 +67,12 @@ import { PopupPageComponent } from "../../../../../platform/popup/layout/popup-p
 import { PopupRouterCacheService } from "../../../../../platform/popup/view-cache/popup-router-cache.service";
 import { BrowserPremiumUpgradePromptService } from "../../../services/browser-premium-upgrade-prompt.service";
 import { BrowserViewPasswordHistoryService } from "../../../services/browser-view-password-history.service";
+import {
+  ROUTES_AFTER_EDIT_DELETION,
+  VaultPopupAfterDeletionNavigationService,
+} from "../../../services/vault-popup-after-deletion-navigation.service";
 import { VaultPopupAutofillService } from "../../../services/vault-popup-autofill.service";
-import { VaultPopupScrollPositionService } from "../../../services/vault-popup-scroll-position.service";
 import { closeViewVaultItemPopout, VaultPopoutType } from "../../../utils/vault-popout-window";
-import { ROUTES_AFTER_EDIT_DELETION } from "../add-edit/add-edit.component";
 import {
   AutofillConfirmationDialogComponent,
   AutofillConfirmationDialogResult,
@@ -155,11 +157,11 @@ export class ViewComponent {
     private popupRouterCacheService: PopupRouterCacheService,
     protected cipherAuthorizationService: CipherAuthorizationService,
     private copyCipherFieldService: CopyCipherFieldService,
-    private popupScrollPositionService: VaultPopupScrollPositionService,
     private archiveService: CipherArchiveService,
     private archiveCipherUtilsService: ArchiveCipherUtilitiesService,
     private domainSettingsService: DomainSettingsService,
     private configService: ConfigService,
+    private afterDeletionNavigationService: VaultPopupAfterDeletionNavigationService,
   ) {
     this.subscribeToParams();
   }
@@ -282,8 +284,7 @@ export class ViewComponent {
       return false;
     }
 
-    this.popupScrollPositionService.stop(true);
-    await this.popupRouterCacheService.back();
+    await this.afterDeletionNavigationService.navigateAfterDeletion(this.routeAfterDeletion);
 
     this.toastService.showToast({
       variant: "success",

apps/browser/src/vault/popup/services/vault-popup-after-deletion-navigation.service.spec.ts

@@ -0,0 +1,123 @@
+import { Location } from "@angular/common";
+import { TestBed } from "@angular/core/testing";
+import { Router } from "@angular/router";
+import { mock, MockProxy } from "jest-mock-extended";
+import { of } from "rxjs";
+
+import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service";
+
+import { PopupRouterCacheService } from "../../../platform/popup/view-cache/popup-router-cache.service";
+import { RouteHistoryCacheState } from "../../../platform/services/popup-view-cache-background.service";
+
+import {
+  ROUTES_AFTER_EDIT_DELETION,
+  VaultPopupAfterDeletionNavigationService,
+} from "./vault-popup-after-deletion-navigation.service";
+import { VaultPopupScrollPositionService } from "./vault-popup-scroll-position.service";
+
+describe("VaultPopupAfterDeletionNavigationService", () => {
+  let service: VaultPopupAfterDeletionNavigationService;
+
+  let router: MockProxy<Router>;
+  let location: MockProxy<Location>;
+  let popupRouterCacheService: MockProxy<PopupRouterCacheService>;
+  let scrollPositionService: MockProxy<VaultPopupScrollPositionService>;
+  let platformUtilsService: MockProxy<PlatformUtilsService>;
+
+  beforeEach(() => {
+    router = mock<Router>();
+    location = mock<Location>();
+    popupRouterCacheService = mock<PopupRouterCacheService>();
+    scrollPositionService = mock<VaultPopupScrollPositionService>();
+    platformUtilsService = mock<PlatformUtilsService>();
+
+    router.navigate.mockResolvedValue(true);
+    platformUtilsService.isFirefox.mockReturnValue(false);
+
+    TestBed.configureTestingModule({
+      providers: [
+        VaultPopupAfterDeletionNavigationService,
+        { provide: Router, useValue: router },
+        { provide: Location, useValue: location },
+        { provide: PopupRouterCacheService, useValue: popupRouterCacheService },
+        { provide: VaultPopupScrollPositionService, useValue: scrollPositionService },
+        { provide: PlatformUtilsService, useValue: platformUtilsService },
+      ],
+    });
+
+    service = TestBed.inject(VaultPopupAfterDeletionNavigationService);
+  });
+
+  describe("navigateAfterDeletion", () => {
+    describe("scroll position reset", () => {
+      it("stops the scroll position service on non-Firefox browsers", async () => {
+        platformUtilsService.isFirefox.mockReturnValue(false);
+
+        await service.navigateAfterDeletion();
+
+        expect(scrollPositionService.stop).toHaveBeenCalledWith(true);
+      });
+
+      it("does not stop the scroll position service on Firefox", async () => {
+        platformUtilsService.isFirefox.mockReturnValue(true);
+
+        await service.navigateAfterDeletion();
+
+        expect(scrollPositionService.stop).not.toHaveBeenCalled();
+      });
+    });
+
+    describe("default route (tabsVault)", () => {
+      it("navigates to the vault tab by default", async () => {
+        await service.navigateAfterDeletion();
+
+        expect(router.navigate).toHaveBeenCalledWith(["/tabs/vault"]);
+      });
+
+      it("navigates to the vault tab when explicitly provided", async () => {
+        await service.navigateAfterDeletion(ROUTES_AFTER_EDIT_DELETION.tabsVault);
+
+        expect(router.navigate).toHaveBeenCalledWith(["/tabs/vault"]);
+      });
+
+      it("does not check popup history", async () => {
+        await service.navigateAfterDeletion(ROUTES_AFTER_EDIT_DELETION.tabsVault);
+
+        expect(popupRouterCacheService.history$).not.toHaveBeenCalled();
+      });
+    });
+
+    describe("non-default route", () => {
+      const historyWithArchive: RouteHistoryCacheState[] = [
+        { url: "/tabs/vault" } as RouteHistoryCacheState,
+        { url: "/archive" } as RouteHistoryCacheState,
+        { url: "/view-cipher" } as RouteHistoryCacheState,
+        { url: "/edit-cipher" } as RouteHistoryCacheState,
+      ];
+
+      it("walks back through history when the route is found", async () => {
+        popupRouterCacheService.history$.mockReturnValue(of(historyWithArchive));
+
+        await service.navigateAfterDeletion(ROUTES_AFTER_EDIT_DELETION.archive);
+
+        // archive is at index 1, current is index 3 (length - 1), so stepsBack = 1 - 3 = -2
+        expect(location.historyGo).toHaveBeenCalledWith(-2);
+        expect(router.navigate).not.toHaveBeenCalled();
+      });
+
+      it("falls back to router.navigate when the route is not in history", async () => {
+        const historyWithoutArchive: RouteHistoryCacheState[] = [
+          { url: "/tabs/vault" } as RouteHistoryCacheState,
+          { url: "/view-cipher" } as RouteHistoryCacheState,
+          { url: "/edit-cipher" } as RouteHistoryCacheState,
+        ];
+        popupRouterCacheService.history$.mockReturnValue(of(historyWithoutArchive));
+
+        await service.navigateAfterDeletion(ROUTES_AFTER_EDIT_DELETION.archive);
+
+        expect(location.historyGo).not.toHaveBeenCalled();
+        expect(router.navigate).toHaveBeenCalledWith(["/archive"]);
+      });
+    });
+  });
+});

apps/browser/src/vault/popup/services/vault-popup-after-deletion-navigation.service.ts

@@ -0,0 +1,76 @@
+import { Location } from "@angular/common";
+import { inject, Injectable } from "@angular/core";
+import { Router } from "@angular/router";
+import { firstValueFrom } from "rxjs";
+
+import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service";
+
+import { PopupRouterCacheService } from "../../../platform/popup/view-cache/popup-router-cache.service";
+
+import { VaultPopupScrollPositionService } from "./vault-popup-scroll-position.service";
+
+/**
+ * Available routes to navigate to after deleting a cipher.
+ * Useful when the user could be coming from a different view other than the main vault (e.g., archive).
+ */
+export const ROUTES_AFTER_EDIT_DELETION = Object.freeze({
+  tabsVault: "/tabs/vault",
+  archive: "/archive",
+} as const);
+
+export type ROUTES_AFTER_EDIT_DELETION =
+  (typeof ROUTES_AFTER_EDIT_DELETION)[keyof typeof ROUTES_AFTER_EDIT_DELETION];
+
+/**
+ * Service that handles navigation after a cipher is deleted.
+ *
+ * When the deletion target route is somewhere other than the default vault tab,
+ * this service walks back through the popup history to find it (preserving the
+ * browser-extension back-button behaviour). If the route is not found in
+ * history it falls back to a normal `Router.navigate`.
+ */
+@Injectable({
+  providedIn: "root",
+})
+export class VaultPopupAfterDeletionNavigationService {
+  private router = inject(Router);
+  private location = inject(Location);
+  private popupRouterCacheService = inject(PopupRouterCacheService);
+  private scrollPositionService = inject(VaultPopupScrollPositionService);
+  private platformUtilsService = inject(PlatformUtilsService);
+
+  /**
+   * Navigate to the appropriate route after a cipher has been deleted.
+   * Resets the vault scroll position on non-Firefox browsers to prevent
+   * auto-scrolling to a stale position. Firefox is excluded because eagerly
+   * clearing scroll state triggers its native scroll restoration, causing
+   * unwanted scroll behavior.
+   *
+   * @param routeAfterDeletion - The target route to navigate to. Defaults to the main vault tab.
+   */
+  async navigateAfterDeletion(
+    routeAfterDeletion: ROUTES_AFTER_EDIT_DELETION = ROUTES_AFTER_EDIT_DELETION.tabsVault,
+  ): Promise<void> {
+    if (!this.platformUtilsService.isFirefox()) {
+      this.scrollPositionService.stop(true);
+    }
+
+    if (routeAfterDeletion !== ROUTES_AFTER_EDIT_DELETION.tabsVault) {
+      const history = await firstValueFrom(this.popupRouterCacheService.history$());
+      const targetIndex = history.map((h) => h.url).lastIndexOf(routeAfterDeletion);
+
+      if (targetIndex !== -1) {
+        const stepsBack = targetIndex - (history.length - 1);
+        // Use historyGo to navigate back to the target route in history.
+        // This allows downstream calls to `back()` to continue working as expected.
+        this.location.historyGo(stepsBack);
+        return;
+      }
+
+      await this.router.navigate([routeAfterDeletion]);
+      return;
+    }
+
+    await this.router.navigate([routeAfterDeletion]);
+  }
+}

apps/browser/src/vault/popup/settings/archive.component.ts

@@ -42,7 +42,7 @@ import {
 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 { ROUTES_AFTER_EDIT_DELETION } from "../components/vault/add-edit/add-edit.component";
+import { ROUTES_AFTER_EDIT_DELETION } from "../services/vault-popup-after-deletion-navigation.service";
 
 // FIXME(https://bitwarden.atlassian.net/browse/CL-764): Migrate to OnPush
 // eslint-disable-next-line @angular-eslint/prefer-on-push-component-change-detection